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

ignore first line when comparing file contents #352

Closed
wants to merge 2 commits into from
Closed

ignore first line when comparing file contents #352

wants to merge 2 commits into from

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Jun 5, 2015

Following @leeper's suggestion in #334.

True, this alone will lead to different versions in the headers over time. But this is really much simpler than maintaining the version information in a dedicated file (where? DESCRIPTION? a new file? NAMESPACE is not necessarily created by us...), and if a generated file is updated, the version info will be updated too.

@hadley
Copy link
Member

hadley commented Jun 5, 2015

I find this approach very distasteful - I'm not entirely sure why, but I really don't like the fact that you'll have different version numbers scattered all over the place.

@krlmlr
Copy link
Member Author

krlmlr commented Jun 5, 2015

The semantics change: It's the version of roxygen2 used when the file was last updated.

Let's consider the use case: A notification that pops up when documentation is built using a different version of roxygen2 than the maintainer's version. But why do we care about this if the output is identical otherwise?

The current state is a real hassle in practice if I choose to use e.g. a development version of roxygen2 and find a gazillion of bogus changes after running document().

How about clarifying this in the comment: "Created by roxygen2, last updated on $date using roxygen2 version $ver"? In addition, I can make this an option (Roxygen: in DESCRIPTION).

Your original approach -- recording the version -- can be implemented independently, perhaps it's possible to write Roxygen: list(version = ...) in the DESCRIPTION.

@hadley
Copy link
Member

hadley commented Jun 5, 2015

I agree that the current system is annoying, but it does have the advantage of ensuring that you always have matching roxygen versions - this is important as it avoids spurious diffs due to differing versions. The current approach generates too much noise, but I think the basic idea is correct - you should record the versions of the packages used in development to make sure that all developers are on the same page.

@krlmlr
Copy link
Member Author

krlmlr commented Jun 7, 2015

If correctness of the (redundant) documentation is important, it can be checked with a testthat test. To avoid different roxygen2 versions in the comments, I could reuse the first line of the original file (with a warning or a message) -- would this be better?

@kirillseva
Copy link

I have to agree, diffs on pull-requests become very spammy if people use different roxygen versions. That's why in our company we have to lock roxygen version, which is much harder to do for open source projects

@hadley
Copy link
Member

hadley commented Jun 8, 2015

I would rather store the roxygen version in a single file. Then spurious diffs are avoided if an update to roxygen2 doesn't change the output, but it's still easy to spot if someone has roxygenised with the wrong version (and hence explains why Rd files have changed). It remains to figure out which is the best file to store that fact in.

@krlmlr
Copy link
Member Author

krlmlr commented Jun 12, 2015

This is broken anyway: NAMESPACE files always get overwritten. Let's go for the single-file solution.

Is it imperative that roxygen2 updates the file itself? An even less intrusive option would be to use an entry in the Roxygen field of DESCRIPTION, and warn on mismatch.

@krlmlr krlmlr closed this Jun 12, 2015
@krlmlr krlmlr deleted the 334-first-line-diff-ignore branch May 11, 2016 19:20
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.

3 participants