lintr
lintr copied to clipboard
r-lib
Use XDG config directory
It would be great if the default lintr config would be preferentially looked up in the appropriate XDG base configuration directory instead of the user home directory. That is, if lintr defaulted to loading ${XDG_CONFIG_HOME-$HOME/.config}/lintr (note lack of leading dot in the filename!) and only if that doesn’t exist, checked for $HOME/.lintr. This avoids cluttering the home directory with config files.
Of course this can already be achieved by appropriately setting the lintr.linter_file option. But defaults matter.
This PR implements the necessary change, and stays 100% backwards compatible. Note that this causes the cyclomatic complexity of the find_config function to exceed the linting threshold. I’m open for suggestions of how to change this — but I’m tempted to call it a false positive (and to add an appropriate nolint comment).
Good idea! However, if I understand correctly rappdirs is particularly geared towards GUI applications, rather than cross-platform tools and/or code library packages. In particular, on macOS, rappdirs::user_config_dir('lintr') returns /Users/‹user›/Library/Application Support/lintr, which is generally not where a command line tool/library configuration would be expected to reside. Apple would probably disagree, but the overwhelming convention for such tools on macOS is to either follow XDG or to dump everything in the home dir.
I think using rappdirs is the best solution if we want to go down this route.
Meanwhile, base R provides tools::R_user_dir("lintr", "config"), with backports providing this function to older R versions.
@krlmlr Are you still interested in this? #1757 is about to change the config search behavior so now would be a good time to also include XDG_CONFIG_HOME if it still seems useful.
@AshesITR (Wrong user tagged) Absolutely, and I had thought I had already submitted an updated PR. My bad! I’ll check the other PR and submit my changes ASAP!
Thanks a ton and sorry for the wrong tag. Maybe best to base against the new PR because it simplifies the code complexity by creating a candidate list. This PR would then be as small as +1 line of code and some documentation around it.
Codecov Report
Merging #460 (3b3f44e) into main (04eecd4) will increase coverage by
0.00%. The diff coverage is100.00%.
@@ Coverage Diff @@
## main #460 +/- ##
=======================================
Coverage 98.87% 98.87%
=======================================
Files 109 109
Lines 4621 4622 +1
=======================================
+ Hits 4569 4570 +1
Misses 52 52
| Impacted Files | Coverage Δ | |
|---|---|---|
| R/settings.R | 97.82% <ø> (ø) |
|
| R/settings_utils.R | 100.00% <100.00%> (ø) |
|
| R/lint.R | 96.78% <0.00%> (+<0.01%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
OK, I’ve added the necessary changes. This includes adding an environment variable to override the lintr config file location; I added this change because I think it belongs here: it is a widespread convention that the location of a configuration file can be overridden by an environment variable.
In the case of ‘lintr’ it made sense (I think) to make this environment variable act as the default value for getOption('lintr.linter_file'), rather than being yet another way of specifying the file location.
@AshesITR At your suggestion I rebased this PR onto #1757; however, this means that the outstanding changes to that PR will probably block this one.
Could you write some tests to see if the XDG config is pulled as expected?
LGTM, thanks! Waiting for #1757 to hit the finish line and then rebasing #460 again.
@AshesITR I’ve rebased the PR against the merged main branch now that #1757 has been merged. Please have a look.