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.

Sign up for GitHub

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

Jump to bottom

Validate config files #2225

Merged
merged 39 commits into from
Oct 11, 2023
Merged

Validate config files #2225

merged 39 commits into from
Oct 11, 2023

Conversation

MichaelChirico
Copy link
Collaborator

@MichaelChirico MichaelChirico commented Oct 3, 2023 •
edited
Loading

Closes #2195.

@MichaelChirico
Copy link
Collaborator Author

@AshesITR PTAL at the updated description of valid exclusions. AFAICT this tested usage was not described till now:

lintr/tests/testthat/test-exclusions.R

Line 88 in 9c49c6e

1, # global exclusions are unnamed

@MichaelChirico
Copy link
Collaborator Author

This is another undocumented usage of exclusions:

lintr/tests/testthat/dummy_packages/clean/lintr_test_config

Line 1 in 9c49c6e

exclusions: "R/default_linter_testcode.R"

Should we break that, or extend the description of a valid exclusions to cover this?

@MichaelChirico
Copy link
Collaborator Author

That form's used here too:

lintr/tests/testthat/test-lint_dir.R

Line 88 in 9c49c6e

cat("exclusions:\n 'exclude-me'\n", file = file.path(the_dir, ".lintr"))

@AshesITR
Copy link
Collaborator

AshesITR commented Oct 4, 2023

This is another undocumented usage of exclusions:

lintr/tests/testthat/dummy_packages/clean/lintr_test_config

Line 1 in 9c49c6e

exclusions: "R/default_linter_testcode.R"

Should we break that, or extend the description of a valid exclusions to cover this?

I wouldn't want to break character vectors of file names to exclude.

AshesITR
AshesITR requested changes Oct 4, 2023
View reviewed changes
R/settings.R Outdated Show resolved Hide resolved
R/settings.R Outdated Show resolved Hide resolved
AshesITR
AshesITR reviewed Oct 4, 2023
View reviewed changes
R/settings.R Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2023 •
edited
Loading

Codecov Report

Merging #2225 (a152731) into main (c6c5b5a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a152731 differs from pull request most recent head 74328be. Consider uploading reports for the commit 74328be to get more accurate results

@@           Coverage Diff           @@
##             main    #2225   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         114      114           
  Lines        5198     5247   +49     
=======================================
+ Hits         5180     5229   +49     
  Misses         18       18
Files Coverage Δ
R/exclude.R 100.00% <ø> (ø)
R/settings.R 99.00% <100.00%> (+0.93%) ⬆️
R/zzz.R 100.00% <ø> (ø)

@r-lib r-lib deleted a comment from AshesITR Oct 4, 2023
@r-lib r-lib deleted a comment from AshesITR Oct 4, 2023
@r-lib r-lib deleted a comment from AshesITR Oct 4, 2023
@AshesITR AshesITR mentioned this pull request Oct 5, 2023
Open
AshesITR
AshesITR reviewed Oct 5, 2023
View reviewed changes
R/settings.R Outdated Show resolved Hide resolved
AshesITR
AshesITR reviewed Oct 9, 2023
View reviewed changes
NEWS.md Outdated Show resolved Hide resolved
R/settings.R Show resolved Hide resolved
tests/testthat/test-lint_package.R Outdated Show resolved Hide resolved
tests/testthat/test-settings.R Outdated Show resolved Hide resolved
R/exclude.R Outdated
#' "3. Exclusions parameter, a list with named and/or unnamed entries. If present, the name is a path relative to",
#' " the config. Moreover, elements have the following characteristics:",
#' " 1. Unnamed elements specify filenames or directories.",
#' " 2. Named elements are numeric or a list. If present, the name is a linter.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the name a filename or a directory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree it's confusing. But "the name is a path relative to the config" above refers to those names, this one refers to the more nested names. Taking another pass.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PTAL

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

MichaelChirico commented Oct 9, 2023 •
edited
Loading

Hmm, {testthat} just landed with an R>=3.6.0 requirement. We may as well make the switch.

Actually, let's just update that one CI suite for now.

AshesITR
AshesITR approved these changes Oct 11, 2023
View reviewed changes
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.

Looks good for now. ?exclude still needs some work (examples, maybe?), but I think that can be a follow-up.

Thanks for all the work and patience!

@AshesITR AshesITR merged commit dfc51ea into main Oct 11, 2023
20 checks passed
@AshesITR AshesITR deleted the validate-config branch October 11, 2023 23:50
@MichaelChirico
Copy link
Collaborator Author

Thanks for all the work and patience!

I'm really happy w the progress! thanks for the thorough review & insistence!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AshesITR AshesITR AshesITR approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate configs
3 participants
@MichaelChirico @AshesITR @codecov-commenter