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

Load config from ~/rubocop/ and $XDG_CONFIG_HOME #6682

Closed

Conversation

tejasbubane
Copy link
Contributor

if $HOME not set

Spec: https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.8.html

Closes: #6662

  • Wrote [good commit messages][1].
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@tejasbubane tejasbubane force-pushed the fix-home-path-arch-linux branch 2 times, most recently from 3e798b9 to 1596be3 Compare January 22, 2019 14:14
@Mange
Copy link
Contributor

Mange commented Jan 26, 2019

In which context would $HOME not be set but a $XDG_CONFIG_HOME is set?

I went here to see if there was an issue for XDG support already. My take on it would be this:

  • Check for $XDG_CONFIG_HOME/rubocop/config.yml and load it if it exists, else go to next step
  • Check for $HOME/.rubocop.yml and load it if it exits.

Then existing users of the dotfile would not see any breakages, but XDG-focused users could move their existing file to the correct location and have it load automatically.

@tejasbubane
Copy link
Contributor Author

@Mange Gotcha, I did it the other way round. You are right, I will change the implementation. Thanks!

@tejasbubane tejasbubane force-pushed the fix-home-path-arch-linux branch 2 times, most recently from 3ed5a1b to 2591f97 Compare January 28, 2019 08:48
@tejasbubane tejasbubane changed the title Load .rubocop.yml from $XDG_CONFIG_HOME Support loading rubocop config from ~/rubocop/ and $XDG_CONFIG_HOME Jan 28, 2019
@tejasbubane tejasbubane changed the title Support loading rubocop config from ~/rubocop/ and $XDG_CONFIG_HOME Load config from ~/rubocop/ and $XDG_CONFIG_HOME Jan 28, 2019
@tejasbubane tejasbubane force-pushed the fix-home-path-arch-linux branch 4 times, most recently from 58d8c61 to e25c915 Compare January 28, 2019 09:41
@tejasbubane
Copy link
Contributor Author

@Mange Fixed. I made some changes, this is the order being checked now:

RuboCop::ConfigLoader
  .configuration_file_for
    when a config file exists in the parent directory
      returns the path to that configuration file
    when multiple config files exist in ancestor directories
      prefers closer config file
    when no config file exists in ancestor directories
      ENV has `XDG_HOME` defined
        config file exists in `XDG_HOME` directory
          returns path to the file in `XDG_HOME` directory
        no config file in `XDG_HOME` but in rubocop directory
          returns path to the file in XDG_HOME/rubocop directory
      when ENV does not have `XDG_HOME` defined
        and a config file exists in home directory
          returns the path to the file in home directory
        and a config file exists in rubocop directory
          returns the path to the file in home/rubocop directory
      and no config file exists in `XDG_HOME` or `HOME` directories
        falls back to the provided default file

@Mange
Copy link
Contributor

Mange commented Jan 28, 2019

I cannot find any references to a XDG_HOME directory in the specification: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

Is it from some other specification, or is it a mistake? Are you mixing up XDG_CONFIG_HOME with HOME?

Normal config path according to the specification would be $XDG_CONFIG_HOME/rubocop/config.yml, aka $HOME/.config/rubocop/config.yml (if XDG_CONFIG_HOME isn't defined).

@tejasbubane
Copy link
Contributor Author

@Mange That was indeed a mistake. I changed it to XDG_CONFIG_HOME.

@Mange
Copy link
Contributor

Mange commented Feb 27, 2019

I think this looks good now overall. I hope the maintainers agree.

@@ -31,17 +31,26 @@ def find_files_upwards(filename, start_dir, use_home: false)
private

def traverse_files_upwards(filename, start_dir, use_home)
traverse_file(filename, start_dir).each { |f| yield(f.to_s) }
return unless use_home && (ENV.key?('HOME') ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems to me that we're mixing different concerns in this method. Ideally the file traversal should be separate from checks for which directory contains the config file.

@@ -15,6 +15,7 @@ class ConfigNotFoundError < Error
# directories are inspected.
class ConfigLoader
DOTFILE = '.rubocop.yml'.freeze
CONFIGFILE = 'rubocop/config.yml'.freeze
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably this should be .rubocop, not rubocop. Usually config dirs are "hidden". I'd also name the constant CONFIG_FILE.

Copy link
Contributor

Choose a reason for hiding this comment

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

Files in XDG_CONFIG_HOME should not be hidden according to specs. The most common convention is appname/config.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 10, 2019

I left a few small comments. Generally I'm fine with the proposed solution.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 3, 2019

I'm closing this PR due to no recent activity. Feel free to re-open it if you ever come back to it.

@Mange
Copy link
Contributor

Mange commented Apr 6, 2019

I re-opened this in #6895.

bbatsov pushed a commit that referenced this pull request Apr 15, 2019
Users now have an additional place they can have their own config file,
if they prefer to not have dotfiles inside their home directory.

Based on initial work by @tejasbubane in #6682.
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