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

stricter test for has_description #702

Merged
merged 9 commits into from
Mar 13, 2021
Merged

stricter test for has_description #702

merged 9 commits into from
Mar 13, 2021

Conversation

MichaelChirico
Copy link
Collaborator

Just got bit by find_package landing on a DESCRIPTION in a parent directory which is a folder.

Changed the has_description test to be a bit stricter.

Just got bit by find_package landing on a DESCRIPTION in a parent directory which is a folder.

Changed the has_description test to be a bit stricter.
@AshesITR
Copy link
Collaborator

Could you add a test, e.g. using a mini Project with file structure

DESCRIPTION # file
dir
  |- DESCRIPTION # directory
  |  \- dummy_file # to have git track the directory
  \- R
      \- some_file.R

And testing the output of find_package(path = "dir/R/some_file.R")?
The root DESCRIPTION could be made a proper DESCRPITION file in dcf should we decide to check even more thoroughly in the future.

R/lint.R Show resolved Hide resolved
@MichaelChirico
Copy link
Collaborator Author

Sorry, I'm not following what we're trying to test exactly. We run lint_package on dir and it finds a lint in some_file, is that right? Don't we need R to be a sibling of the "correct" DESCRIPTION then?

@AshesITR
Copy link
Collaborator

AshesITR commented Jan 2, 2021

I would keep the test atomic and only test that find_package returns the correct (expected) package directory.
No lints expected for this test IINM.

@MichaelChirico
Copy link
Collaborator Author

I was thinking to run lint_package to get at the behaviour through the "public" (exported) API, but of course we could directly test find_package as well. WDYT

@AshesITR
Copy link
Collaborator

AshesITR commented Jan 2, 2021

Ah I see what you're getting at. That's a good point, would also be okay. In that case the DESCRIPTION directory should have a second-degree sibling directory named R which contains sources with lints.

@AshesITR
Copy link
Collaborator

AshesITR commented Feb 3, 2021

@MichaelChirico Do you want to add a test for this so we can ship it with 3.0.0?
The change itself looks good to me.

@MichaelChirico MichaelChirico added this to the 3.0.0 milestone Feb 3, 2021
Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

LGTM

@AshesITR AshesITR merged commit 8fb3872 into master Mar 13, 2021
@AshesITR AshesITR deleted the find_package_strict branch March 13, 2021 09:35
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.

None yet

3 participants