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

Automatically add .lintr to .Rbuildignore by the use_lintr function #1895

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

eitsupi
Copy link
Contributor

@eitsupi eitsupi commented Jan 8, 2023

Close #1805

It would be useful if .lintr files were automatically added to .Rbuildignore when creating R packages.

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2023

Codecov Report

Merging #1895 (9bbfb99) into main (9af53f1) will decrease coverage by 0.79%.
The diff coverage is 100.00%.

❗ Current head 9bbfb99 differs from pull request most recent head 7f5ae47. Consider uploading reports for the commit 7f5ae47 to get more accurate results

@@            Coverage Diff             @@
##             main    #1895      +/-   ##
==========================================
- Coverage   99.65%   98.86%   -0.79%     
==========================================
  Files         113      112       -1     
  Lines        5166     4856     -310     
==========================================
- Hits         5148     4801     -347     
- Misses         18       55      +37     
Files Changed Coverage Δ
R/use_lintr.R 100.00% <100.00%> (ø)

... and 71 files with indirect coverage changes

@eitsupi eitsupi force-pushed the buildignore-dotlintr branch from 537b3f0 to 780cb2b Compare January 8, 2023 04:55
@eitsupi
Copy link
Contributor Author

eitsupi commented Jan 8, 2023

Is purrr causing the R 3.4 test to fail, but probably not a problem? #1870 (comment)

@eitsupi eitsupi force-pushed the buildignore-dotlintr branch 2 times, most recently from db34ae5 to 780cb2b Compare January 8, 2023 14:50
@AshesITR
Copy link
Collaborator

AshesITR commented Jan 8, 2023

The R 3.4 build error can be fixed by ignoring packages that import purrr for R < 3.5.

However, not a big fan of introducing a dependency on usethis and fs just for this feature.
Can you add the line to .Rbuildignore using base R?

@eitsupi eitsupi force-pushed the buildignore-dotlintr branch 3 times, most recently from a96d166 to 49e1acd Compare January 9, 2023 05:53
R/use_lintr.R Outdated Show resolved Hide resolved
R/use_lintr.R Outdated Show resolved Hide resolved
@eitsupi eitsupi force-pushed the buildignore-dotlintr branch from 49e1acd to efe56d1 Compare January 9, 2023 06:01
@eitsupi
Copy link
Contributor Author

eitsupi commented Jan 9, 2023

It doesn't seem to pass the test but locally it does so I don't know what is causing it.

R/use_lintr.R Outdated
@@ -43,5 +43,29 @@ use_lintr <- function(path = ".", type = c("tidyverse", "full")) {
)
)
write.dcf(the_config, config_file, width = Inf)

# If this is an R package and if available, add .lintr file to .Rbuildignore
if (file.exists("DESCRIPTION") && file.exists(".Rbuildignore")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test looks in the current working directory, not where the config was placed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since there is no guarantee that DESCRIPTION and .lintr will be placed in the same directory, I believe that the current working directory can should be used.

@eitsupi
Copy link
Contributor Author

eitsupi commented Jan 9, 2023

Can you add the line to .Rbuildignore using base R?

Sorry, finally I could not get this to work without usethis.
The function I wrote in base R worked locally but did not pass the test on GitHub.
It is possible that the test is poorly written.

@Bisaloo
Copy link
Contributor

Bisaloo commented Jan 19, 2023

Noting that this PR will fix #1805.

@eitsupi
Copy link
Contributor Author

eitsupi commented Jan 19, 2023

Noting that this PR will fix #1805.

Oh, I didn't realize it was already requested. Thank you for pointing that out!

@MichaelChirico
Copy link
Collaborator

I agree about not adding either dependency.

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.

use_lintr() should add .lintr to .Rbuildignore
6 participants