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

Feature: add support for RUBOCOP_CACHE_ROOT env variable to control cache path #8639

Merged
merged 14 commits into from
Sep 6, 2020
Merged

Feature: add support for RUBOCOP_CACHE_ROOT env variable to control cache path #8639

merged 14 commits into from
Sep 6, 2020

Conversation

sascha-wolf
Copy link
Contributor

@sascha-wolf sascha-wolf commented Sep 3, 2020

This PR introduces the --cache-root command line option and support for the RUBOCOP_CACHE_ROOT env variable to control the cache root directory.

It takes precedence over everything else (even the config file) which makes it perfectly suited for CI usage where it's very helpful to control the cache path (without interference from the config file) to consistently be able to save and restore rubocop's cache files.

Possible further steps

  • also add a cli option --cache-root
  • print a warning when there is both: RUBOCOP_CACHE_ROOT and CacheRootDirectory in the config file

TODO

  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@sascha-wolf sascha-wolf changed the title Feature/add env to control cache path Feature: add support for RUBOCOP_CACHE_ROOT env variable to control cache path Sep 3, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Sep 4, 2020

I'm fined with the proposed change, although the rationale for it in the docs should be expanded upon. I also like the idea a having a cli option for this, which IMO should have the highest possible priority (and also introduces less cognitive overhead).

@sascha-wolf
Copy link
Contributor Author

@bbatsov I've added a --cache-root CLI option in 96e215d, dccc65b, and 293c936.

although the rationale for it in the docs should be expanded upon

What exactly do you mean with this? Do you mean that the docs should also mention the CI use-case?

@sascha-wolf
Copy link
Contributor Author

On another note, do you think it would be beneficial to print a warning, if either --cache-root or RUBOCOP_CACHE_ROOT take precedence while other configuration exists?

@sascha-wolf
Copy link
Contributor Author

@bbatsov I've expanded the docs to include the --cache-root command line option and list the options to control the cache root in order of precedence (bfcea67).

Is this what you had in mind?

@sascha-wolf sascha-wolf marked this pull request as ready for review September 4, 2020 08:55
@bbatsov
Copy link
Collaborator

bbatsov commented Sep 6, 2020

On another note, do you think it would be beneficial to print a warning, if either --cache-root or RUBOCOP_CACHE_ROOT take precedence while other configuration exists?

Might be useful, but I don't think that will add that much value, as in some cases the users would have done this intentionally and the warning would just be annoying to them (e.g. different setup for local development and CI with the same config file).

@bbatsov bbatsov merged commit dfdf8f9 into rubocop:master Sep 6, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Sep 6, 2020

Thanks!

@sascha-wolf sascha-wolf deleted the feature/add-env-to-control-cache-path branch September 7, 2020 08:26
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