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

Tests for update collate #304

Closed
wants to merge 6 commits into from

Conversation

Geoff99
Copy link
Contributor

@Geoff99 Geoff99 commented Nov 16, 2014

This relates to pull request #303 which should resolve issue #302.

The change proposed in this pull request is the addition of a testthat in the roxygen2 test suite to check that update_collate actually does do nothing if no @includes are found (and in particular leaves any manually entered Collate field unaltered).

The test uses a template package called testCollateNoInclude, the key feature of which is that the DESCRIPTION file has a Collate field which was entered manually rather than generated by roxygen2 as a consequence of @include directives. The .r files in the R directory do not have any @includes.

The template package is copied into tempdir() before running the tests, since update_collate can have permanent side effects (viz generating a Collate field in the DESCRIPTION file). If update_collate was run on the template package itself, rather than on the copy in tempdir(), and the test failed, the template package would be altered, and future runs of the test would not be testing what the test is supposed to test.

I had intended to make this entirely separate from the fix mentioned in pull request #303 , but due to my learner level of competence with git this branch also includes the fix itself. If that is a problem, let me know and I will recreate it as an independent branch in the way I originally intended.

Feedback and guidance (re the pull request and/or my usage of git) most appreciated.

Geoff

to reflect the fix that makes it truly do nothing, instead of creating a blank Collate field when not \code{@@includes} are found.
The test will create a temporary copy of this package, run update_collate, and see what happens.  A temporary copy is needed so that if  update_collate makes changes, future runs of this testthat are not affected.
So I can reference this updated version in the DESCRIPTION Imports field of my devtools/update_collate fork
@hadley hadley closed this in e579e82 Dec 9, 2014
@hadley
Copy link
Member

hadley commented Dec 9, 2014

Thanks! That was very helpful.

@Geoff99
Copy link
Contributor Author

Geoff99 commented Dec 11, 2014

Really pleased to have been able to make a tiny contribution to a great product.

Geoff

PS If it’s OK I’ll reopen a pull request on devtools (about being able to load packages where the definition of S4 classes depend on a non-alphabetic Collate order generated by @includes) that needed this fix to roxygen2 before it would work. But not until I’ve got my forked copies up to date again, and made sure the devtools fix is still needed.

From: Hadley Wickham [mailto:[email protected]]
Sent: Wednesday, 10 December 2014 8:16 AM
To: klutometis/roxygen
Cc: Geoff99
Subject: Re: [roxygen] Tests for update collate (#304)

Thanks! That was very helpful.


Reply to this email directly or view it on GitHub #304 (comment) . https://github.com/notifications/beacon/AH9DBkQObIwXK2O5i93sMJuA86uSgpBKks5nV14kgaJpZM4C74x2.gif

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.

2 participants