Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix r cmd check note about #! interpreter line #83

Merged
merged 3 commits into from
Dec 3, 2020

Conversation

jgabry
Copy link
Member

@jgabry jgabry commented Dec 2, 2020

Fixes #82.

Checks if first line is #! interpreter line and if so puts the "don't edit by hand" message on the second line instead of the first. This avoids the new r cmd check note reported in #82.

@jgabry jgabry requested review from bgoodri and mlysy December 2, 2020 16:51
Copy link
Collaborator

@bgoodri bgoodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but does anything that was grepping for "do not edit this file" foolishly assume that such a line could only appear at the top of the file?

@jgabry
Copy link
Member Author

jgabry commented Dec 2, 2020

Hmm, that's definitely something we could have foolishly done but I don't actually think we are grepping for that anywhere. I may have missed it though. Where would we need to do that?

@bgoodri
Copy link
Collaborator

bgoodri commented Dec 2, 2020 via email

@jgabry
Copy link
Member Author

jgabry commented Dec 2, 2020

Ah ok yeah. I'll check on that.

@jgabry
Copy link
Member Author

jgabry commented Dec 2, 2020

@bgoodri You were right. In two places I just changed:

readLines(dest_file, n = 1) == noedit_msg

to

noedit_msg %in% readLines(dest_file, n = 5)

@bgoodri bgoodri merged commit ce537b1 into master Dec 3, 2020
@mlysy
Copy link
Collaborator

mlysy commented Dec 3, 2020

The proposed fix should be fine. I'm pretty sure I was just checking the first line each time.

More long-term, the current auto_config mechanism strikes me as less than ideal.

First of all, .warning_nooverwrite() in R/rstan_package_utils.R issues a warning if existing files won't be overwritten. But files such as src/Makevars may need to be modified depending on the developer's needs. Since auto_config = TRUE runs rstan_config() during installation, this will issue an R CMD check warning such that auto_config needs to be disabled. A quick fix is to change the warning to a message.

A second less important issue is that it's cumbersome to change the structure of the package based on whether or not auto_config is enabled.

What I was thinking instead was to create a hook to run rstan_config() as part of devtools::document(). It seems possible to do this with roxygen, but I haven't experimented much with this approach.

@jgabry
Copy link
Member Author

jgabry commented Dec 7, 2020

@mlysy Thanks, yeah I think changing the auto_config mechanism like sounds good if you want to try that out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

configure files in packages made with rstan_create_package() produces a note when running R CMD check
3 participants