SublimeLinter / SublimeLinter-rubocop

SublimeLinter 3 plugin for Ruby, using rubocop.
MIT License
159 stars 40 forks source link

Try to infer rubocop config #71

Closed motine closed 4 years ago

motine commented 4 years ago

Since we are using the STDIN to pipe the contents of the editor in rubocop can not infer the path of the .rubocopo.yml within the current project. It falls back to the user's default (in home dir). Hence, I added a little snippet which tries to find the config.

Also see Rubocop's config path logic.

kaste commented 4 years ago

Well that looks like you're fixing a bug in rubocop. Since we provide the filename via the stdin flag, we assume rubocop will use that name for config resolution. This is also stated in the comment here in the code.

Otherwise, the code looks good.

motine commented 4 years ago

Cool, I just fixed the code style issues.

kaste commented 4 years ago

Oh, you misunderstood. First paragraph you linked

RuboCop will start looking for the configuration file in the directory where the inspected file is and continue its way up to the root directory.

And their code e.g. https://github.com/rubocop-hq/rubocop/blob/8767ed070f276403d94b5c821c54ad635b7d4b41/lib/rubocop/options.rb#L35-L38

The point is we set --stdin to a filename, and then rubocop should find config files going upwards. (Basically just what you do here.)

motine commented 4 years ago

mmm, well... the thing is, that the inference of the config in the project dir does not work. how do you think we should proceed?

kaste commented 4 years ago

Digging deeper.

You could enable debug mode for SublimeLinter. Then open up Sublime's console to see it. Note the command plus working dir SublimeLinter runs for a particular file. Try to reproduce that command on the command line. It should behave the same. Does it work differently if instead of passing the file via stdin, you just pass the filename... Then we will sure find out if we're doing it wrong, or if there is an issue with rubocop itself.

motine commented 4 years ago

alright, I tried this. assuming that I have /myproject/.rubocop.yml and run rubocop --stdin /myproject/app/thing.rb rubocop will fall back to the rubocop config in my home dir. Hence, you are right, that rubocop does not consider the path. Subsequently, would it be possible to merge this request?

motine commented 4 years ago

I checked the docs of rubocop again and I might have found a hint:

Pipe source from STDIN. This is useful for editor integration. Takes one argument, a path, relative to the root of the project. RuboCop will use this path to determine which cops are enabled (via eg. Include/Exclude), and so that certain cops like Naming/FileName can be checked.

I am pretty sure that the path argument is only used to include or exclude certain files. Yet, it is not used to infer the config path.

What do you think, can we merge this thing then?

kaste commented 4 years ago

I cannot reproduce this. This is on Windows using the latest(?) rubocop 0.85.1. I just made a fresh project. rubocop --init in the project root, then mkdir sub, enter some garbage in sub/foo.rb

> type sub\foo.rb | C:\Dev\Ruby26-x64\bin\rubocop.BAT --format emacs --force-exclusion --stdin D:\rtest\sub\foo.rb
D:/rtest/sub/foo.rb:1:1: C: Style/FrozenStringLiteralComment: Missing frozen string literal comment.

Then edit .rubocop.yml

> type .rubocop.yml
AllCops:
  NewCops: enable

Style/FrozenStringLiteralComment:
  Enabled: false

Running rubocop again:

> type sub\foo.rb | C:\Dev\Ruby26-x64\bin\rubocop.BAT --format emacs --force-exclusion --stdin D:\rtest\sub\foo.rb
motine commented 4 years ago

Which path were you in when you did your tests? Can you try on a Mac or Linux?

Either way, I can assure you that it does not work on my Mac. I think change does not hurt and would help me and other users. I would appreciate if you could merge the request. (and don't get me wrong, I appreciate your diligence).

kaste commented 4 years ago

cwd is the project root I created, basically where .rubocop.yml is.

motine commented 4 years ago

Ah understand. I think this is why you can't reproduce it. I guess sublime does not give us the courtesy to change the directory for us. Merge?

kaste commented 4 years ago

I can't reproduce it going one dir up.

|| D:\ =
> type rtest\sub\foo.rb | C:\Dev\Ruby26-x64\bin\rubocop.BAT --format emacs --force-exclusion --stdin D:\rtest\sub\foo.rb

I also looked what vim-ale does here, and they do the exact same thing.

Anyway, it is possible to set the working dir, and you can also set --config manually, or semi-automatic because we expand some variables. Please look at the SublimeLinter settings.

motine commented 4 years ago

Alright, thanks for your input. I will not continue to investigate this. The proposed solution works and is in line with the robocop config options (and meaning). Please merge it.

kaste commented 4 years ago

Doesn't look like I'll merge this, does it? I don't think it's inline with the rubocop docs and how I understand their code. I might be wrong but couldn't reproduce either.

Now the PR is not to merge anyway as is because it would break the possibility for the users to manually set the config file, overruling the automatic.

And please don't finish every comment with can we merge.

motine commented 4 years ago

The critique with the config file override is indeed valid. Does it make sense to fix it or will you insist on reproducing it under windows?

kaste commented 4 years ago

At this point, since you can reproduce your issue on the command line, it makes more sense to open an issue at the nice rubocop repo.

It makes no sense to assume different behavior on the different platforms as intended.

motine commented 4 years ago

@ckhall, @aparajita, @reconbot, what do you think about this issue?

motine commented 4 years ago

no feedback from the other collaborators, @ckhall, @aparajita, @reconbot?

aparajita commented 4 years ago

@motine I haven't been involved with the project for many years, no opinion here.

reconbot commented 4 years ago

Same

On Tue, Jun 30, 2020 at 12:07 PM Aparajita Fishman notifications@github.com wrote:

@motine https://github.com/motine I haven't been involved with the project for many years, no opinion here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SublimeLinter/SublimeLinter-rubocop/pull/71#issuecomment-651892838, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGK3WOG23YLDGWKDWYZC3RZIET7ANCNFSM4OCOZ3BA .

--

Francis Gulotta wizard@roborooter.com

motine commented 4 years ago

Alright, @kaste you have won. I give up. You have successfully put enough obstacles in my way (e.g. I have to reproduce it under Windows), that I am not able to contribute. I am sad.