Check for .lintr file in extra subdirectory #1757

tonyk7440 · 2022-10-28T16:12:09Z

Added another condition to check for the .lintr file in the .github/linters subdirectory to improve compatibility with Super-Linter.

Let me know if there's anything I missed, happy to update

AshesITR · 2022-10-28T16:15:23Z

If we consider this, we might also want #460

tonyk7440 · 2022-10-28T16:47:26Z

I can see there is a lint error raised for the cyclomatic complexity linter. I'm not sure what would be the best way to resolve. I don't see an obvious way to make the code simpler. I could increase the threshold to be 16 but not sure if that is an advised route.

Hence marking as ready for review

AshesITR · 2022-10-28T18:02:23Z

While the current search order is non-breaking, it seems very counter-intuitive to give a "global" config at ~/.lintr priority over a local config at .github/linters.

AshesITR · 2022-10-29T12:52:11Z

Regarding the cyclomatic complexity: You could refactor the existence check into a for loop that returns as soon as one of the candidate files exists.

codecov-commenter · 2022-10-31T16:46:45Z

Codecov Report

Merging #1757 (581e51d) into main (c6099d5) will decrease coverage by 0.46%.
The diff coverage is 100.00%.

❗ Current head 581e51d differs from pull request most recent head e0ccfbb. Consider uploading reports for the commit e0ccfbb to get more accurate results

@@            Coverage Diff             @@
##             main    #1757      +/-   ##
==========================================
- Coverage   98.95%   98.49%   -0.46%     
==========================================
  Files         113      113              
  Lines        4953     4990      +37     
==========================================
+ Hits         4901     4915      +14     
- Misses         52       75      +23

Impacted Files	Coverage Δ
R/extract.R	`98.68% <100.00%> (+0.15%)`	⬆️
R/function_left_parentheses_linter.R	`100.00% <100.00%> (ø)`
R/indentation_linter.R	`100.00% <100.00%> (ø)`
R/lint.R	`96.75% <100.00%> (ø)`
R/linter_tags.R	`97.50% <100.00%> (+0.06%)`	⬆️
R/lintr-deprecated.R	`100.00% <100.00%> (ø)`
R/object_usage_linter.R	`99.40% <100.00%> (+<0.01%)`	⬆️
R/settings_utils.R	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

…l file

tonyk7440 · 2022-10-31T18:47:44Z

@AshesITR Thanks for the pointers, I have updated the code, it looks to be passing that and other tests now. Let me know if you suggest other changes

AshesITR

@tonyk7440 thanks for your work so far!

@jimhester, @MichaelChirico, @IndrajeetPatil: WDYT about expanding the config search paths?

R/settings_utils.R

jimhester · 2022-11-03T12:41:59Z

Sounds good to me

AshesITR

Nice work so far, thanks!
Now that we are sure we'll accept this change, please add a test package where the .github/linters/.lintr config takes effect.
Let me know if you need hints on how to do that.

NEWS.md

vignettes/lintr.Rmd

AshesITR · 2022-11-04T19:21:29Z

Thanks for trying to cook up a test.
Due to the R CMD check notes, you'll likely have to programmatically dir.create() the .github path.
Also note we usually use a different lintr.linter_file option to circumvent said R CMD check note regarding the .lintr filename.
withr::local_options(lintr.linter_file = "lintr_test_config")

Finally, to test that the config is pulled from .github/linters, you should make sure the test package does not contain a linter config at its root.
That way, if the config is not found, no exclusions would take effect and the test would fail when comparing to the lints generated with the linter config file present at the package root.

Right now, IINM, the lint_package() calls in your test always find .lintr at the package root.

tonyk7440 · 2022-11-04T19:53:51Z

Thanks @AshesITR!

I've updated the code, seems like the tests are passing now, let me know if I should make further changes.

AshesITR

Excellent work so far, thank you!
Just a few more adjustments.

Regarding the NEWS wording, I'd like to have input from @MichaelChirico.

R/settings_utils.R

tests/testthat/test-lint_package.R

tonyk7440 · 2022-11-15T18:14:11Z

Hi @AshesITR , sorry for the delay, currently very busy with work, will try to get some time this coming weekend

AshesITR · 2022-11-15T21:43:31Z

No worries, thanks for your feedback and good work so far.

klmr · 2023-03-16T23:15:49Z

@tonyk7440 Hi, just wondering: are there any updates on this PR?

MichaelChirico · 2023-03-29T23:48:07Z

Gentle ping @tonyk7440 as we're moving towards release and it would be ideal to include this breaking change alongside a few others at the bump to 3.1.0.

If you're currently short on time, please ping & we can try and crowdsource getting this across the finish line.

Thanks again!

tonyk7440 · 2023-04-14T16:46:36Z

Hi @AshesITR @MichaelChirico @klmr , sorry for the delay in responding.
I've resolved most of the comments above. There is one outstanding ask from @AshesITR, due to my track record above I'll ask to crowdsource this change. Or else the above changes could be merged for the upcoming release and postpone that change to a later release?
Thanks!

klmr · 2023-04-20T20:22:02Z

R/settings_utils.R

+
+  # Search through locations, return first valid result
+  for (loc in file_locations) {
+    if (isTRUE(file.exists(loc))) {


Why the isTRUE() call? As far as I can tell file.exists() should always return exactly one logical value here so this code doesn’t need to guard against non-logical values or vectors of length > 1.

Ugh, this comment (along with others that seem to be gone?!) was written months ago but was somehow part of a pending review without my noticing.

Previously the test case never failed, even without the relevant patch.

This test will currently fail, and should no longer fail once `.github/linters` is searched in parent directories as well as in the current directory.

klmr · 2023-04-27T22:42:23Z

Alright, I’ve pushed my suggested changes. Happy for a review.

AshesITR · 2023-04-28T04:48:32Z

vignettes/lintr.Rmd

-3.  A linter file in a parent directory of the currently-searched directory, starting from the deepest path, moving upwards one level at a time.
-4.  A linter file in the user's `HOME` directory.
+3.  A linter file in the `.github/linters` child directory of the currently-searched directory.
+4.  A linter file in a parent directory of the currently-searched directory, starting from the deepest path, moving upwards one level at a time.


This could be made more clear, stating that 2. and 3. are searched from the parent.

Aren't we checking 2. and 3. for the current directory and only if none match, the parent is searched recursively?

This description suggests we first search for .lintr in any parent and only look for .github/linters/.lintr if there is no .lintr anywhere in the path.

@klmr this is the only blocker I see. Good to merge once the docs match the behavior.

Oh I completely missed these comments. I agree, my description is incorrect. Will change.

Updated. Please take a look. I think we won’t get around making nested bullet points, since that reflects how the algorithm works. Is this clear enough?

AshesITR · 2023-04-28T04:52:43Z

NEWS.md

@@ -2,6 +2,7 @@

 ## Deprecations & Breaking Changes

+* .lintr configs can now be kept in the directory .github/linters for better compatibility with Super-Linter. Note that this may be a breaking change if you already have a config in both such a directory and in your R project's root since the former will now be discovered first where it was ignored before. Please see vignette("lintr") for details on how configs are discovered. (#1746, @tonyk7440)


IINM, the only breaking constellation is if .github/linter/.lintr is in a subdirectory, right?

i.e.

.lintr tests/.github/linters/.lintr

will find a different config when run from tests/. Is that correct?

Also, you should add your name, @klmr.

You’re absolutely right! Will amend.

Fixed the description based on review, performed very minor copy editing, and adapted formatting to be consistent.

AshesITR

LGTM. Just a minor nitpick left.

AshesITR · 2023-05-10T04:39:47Z

vignettes/lintr.Rmd

+2. A project-local linter file; that is, either
+    1. a linter file (that is, a file named like `lintr.linter_file`) in the currently-searched directory, i.e. the directory of the file passed to `lint()`; or
+    2. a linter file in the `.github/linters` child directory of the currently-searched directory.
+3. A project-local linter file in the closest parent directory of the currently-searched directory, starting from the deepest path, moving upwards one level at a time. When run from `lint_package()`, this directory can differ for each linted file.


nit: Whitespace is inconsistent across top-level bullets.

Oops. Fixed.

AshesITR

Thanks a lot, you two!

MichaelChirico · 2023-05-22T05:26:29Z

@AshesITR it looked like this is ready to merge, LMK if that was in error.

check for .lintr file in extra subdirectory

27091b9

r-lib#1746 update news

297136f

AshesITR added breaking change ☠️ API change likely to affect existing code and removed breaking change ☠️ API change likely to affect existing code labels Oct 28, 2022

r-lib#1746 update vignette

d3d0be1

tonyk7440 marked this pull request as ready for review October 28, 2022 16:45

Merge branch 'r-lib:main' into update_file_algo

936c451

r-lib#1757 reduce cyclomatic complexity, move local check above globa…

84d9746

…l file

AshesITR reviewed Nov 1, 2022

View reviewed changes

R/settings_utils.R Outdated Show resolved Hide resolved

R/settings_utils.R Outdated Show resolved Hide resolved

r-lib#1746 updates from review

c493fd1

tonyk7440 requested a review from AshesITR November 3, 2022 18:25

AshesITR reviewed Nov 3, 2022

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

vignettes/lintr.Rmd Outdated Show resolved Hide resolved

AshesITR mentioned this pull request Nov 3, 2022

Use XDG config directory #460

Merged

tonyk7440 added 2 commits November 4, 2022 18:16

r-lib#1746 correct vignette order, update news bullet

3985553

r-lib#1746 try adding test

ef46aa3

r-lib#1746 tests, programatically create .github dir

290a9d1

tonyk7440 requested a review from AshesITR November 4, 2022 19:53

AshesITR reviewed Nov 4, 2022

View reviewed changes

R/settings_utils.R Show resolved Hide resolved

tests/testthat/test-lint_package.R Outdated Show resolved Hide resolved

tonyk7440 added 3 commits November 6, 2022 19:09

r-lib#1746 remove .lintr settings file

50fec27

r-lib#1746 better cleanup, use file_path better

7fb1087

r-lib#1746 one more file_path update

3618fd3

AshesITR added this to the 3.0.3 milestone Dec 5, 2022

IndrajeetPatil modified the milestones: 3.0.3, 3.1.0 Dec 5, 2022

Merge branch 'main' into update_file_algo

30527a3

Merge branch 'main' into update_file_algo

83e0155

MichaelChirico modified the milestones: 3.1.0, 3.1.1 Apr 7, 2023

tonyk7440 added 3 commits April 14, 2023 16:26

Merge branch 'main' into update_file_algo

59aee4c

r-lib#1746 updates from review comments

1f4f55d

r-lib#1746 update news per comment

e8353b8

klmr reviewed Apr 20, 2023

View reviewed changes

klmr added 4 commits April 21, 2023 18:50

Fix the test case for r-lib#1757

00a9db9

Previously the test case never failed, even without the relevant patch.

Add failing test for subdirs

0c5f63a

This test will currently fail, and should no longer fail once `.github/linters` is searched in parent directories as well as in the current directory.

Search for .github/linters in parent directories

5d99a14

Remove unnecessary code

2a28c98

AshesITR reviewed Apr 28, 2023

View reviewed changes

klmr added 2 commits April 28, 2023 18:33

Amend news and lintr config search description

be17eba

Fixed the description based on review, performed very minor copy editing, and adapted formatting to be consistent.

Attempt to make description clearer

e0ccfbb

AshesITR reviewed May 10, 2023

View reviewed changes

Fix indentation

6e992f9

AshesITR approved these changes May 11, 2023

View reviewed changes

MichaelChirico merged commit 4ef15f9 into r-lib:main May 22, 2023

Bisaloo mentioned this pull request May 22, 2023

Move lintr config file to .github/linters epiverse-trace/packagetemplate#54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for .lintr file in extra subdirectory #1757

Check for .lintr file in extra subdirectory #1757

tonyk7440 commented Oct 28, 2022

AshesITR commented Oct 28, 2022

tonyk7440 commented Oct 28, 2022 •

edited

Loading

AshesITR commented Oct 28, 2022

AshesITR commented Oct 29, 2022

codecov-commenter commented Oct 31, 2022 •

edited

Loading

tonyk7440 commented Oct 31, 2022

AshesITR left a comment

jimhester commented Nov 3, 2022

AshesITR left a comment •

edited

Loading

AshesITR commented Nov 4, 2022 •

edited

Loading

tonyk7440 commented Nov 4, 2022

AshesITR left a comment •

edited

Loading

tonyk7440 commented Nov 15, 2022

AshesITR commented Nov 15, 2022

klmr commented Mar 16, 2023

MichaelChirico commented Mar 29, 2023

tonyk7440 commented Apr 14, 2023

klmr Apr 20, 2023

klmr Apr 20, 2023

klmr commented Apr 27, 2023

AshesITR Apr 28, 2023

AshesITR Apr 28, 2023

AshesITR May 8, 2023

klmr May 9, 2023

klmr May 9, 2023

AshesITR Apr 28, 2023

AshesITR Apr 28, 2023

klmr Apr 28, 2023

AshesITR left a comment

AshesITR May 10, 2023

klmr May 11, 2023

AshesITR left a comment

MichaelChirico commented May 22, 2023

		@@ -2,6 +2,7 @@

		## Deprecations & Breaking Changes

		* .lintr configs can now be kept in the directory .github/linters for better compatibility with Super-Linter. Note that this may be a breaking change if you already have a config in both such a directory and in your R project's root since the former will now be discovered first where it was ignored before. Please see vignette("lintr") for details on how configs are discovered. (#1746, @tonyk7440)

Check for .lintr file in extra subdirectory #1757

Check for .lintr file in extra subdirectory #1757

Conversation

tonyk7440 commented Oct 28, 2022

AshesITR commented Oct 28, 2022

tonyk7440 commented Oct 28, 2022 • edited Loading

AshesITR commented Oct 28, 2022

AshesITR commented Oct 29, 2022

codecov-commenter commented Oct 31, 2022 • edited Loading

Codecov Report

tonyk7440 commented Oct 31, 2022

AshesITR left a comment

Choose a reason for hiding this comment

jimhester commented Nov 3, 2022

AshesITR left a comment • edited Loading

Choose a reason for hiding this comment

AshesITR commented Nov 4, 2022 • edited Loading

tonyk7440 commented Nov 4, 2022

AshesITR left a comment • edited Loading

Choose a reason for hiding this comment

tonyk7440 commented Nov 15, 2022

AshesITR commented Nov 15, 2022

klmr commented Mar 16, 2023

MichaelChirico commented Mar 29, 2023

tonyk7440 commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klmr commented Apr 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshesITR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshesITR left a comment

Choose a reason for hiding this comment

MichaelChirico commented May 22, 2023

tonyk7440 commented Oct 28, 2022 •

edited

Loading

codecov-commenter commented Oct 31, 2022 •

edited

Loading

AshesITR left a comment •

edited

Loading

AshesITR commented Nov 4, 2022 •

edited

Loading

AshesITR left a comment •

edited

Loading