Fix rubocop linting files excluded by config

[jonknapp]

I was having issues with Rubocop excluding specific files from linter runs and found that the node process that is being spawned is having it's command directory set to the dirname of the file being linted instead of the root directory of the project.

I dug around in the source and found commit 2e42d02 which seems to swap the precedence so that the sourceDir is used before the rootPath.

This flips precedence back so that the --force-exclusion will correctly ignore errors for files excluded from Rubocop that are relative to the project root.

It should be noted I am unsure of the full effect of this change on other linters or why the flip was made in the first place.

The following is a minimal config I use to run Rubocop file exclusions specified in another gem's config:

{
  "ruby.lint": {
    "rubocop": {
      "forceExclusion": true
    }
  },
  "ruby.useBundler": true
}

Example .rubocop.yml file:

inherit_gem:
  rubocop_coffeeandcode:
    - config/default.yml
    - config/rails.yml

inherit_mode:
  merge:
    - Exclude

[wingrunr21]

This was introduced in #232. The reasoning for the change was given there.

[jonknapp]

Thanks for the fast reply and link to the original pull request and reasoning. 👍 I had a feeling the reason for the change was to accommodate a nested config / dependency file. (FYI, It looks like the "upgrade path" link in #232 is no longer resolving to code display.)

I believe that #188 should be solved with multi-root workspace support instead of the current implementation.

To support a monorepo setup outside of VS Code, a user would need to traverse into each of their folders (repo/foo and repo/bar) to run separate bundle exec rubocop ... commands. That would make the root path of those commands different. Inside of VS Code, we need a way to tell which "root" they would like to execute the command from, which I believe multi-root workspace provides.

That would allow a user to open up two root folders in the same workspace and have the linter / format commands run from the appropriate workspace root so that the correct version of Rubocop is used.

If you'd like, I can attempt to add in multi-workspace support to linter.js in this PR to meet this requirement. I am still confused when the fallback to sourceDir is needed though. (maybe it's not 🤷‍♂️ )

I could attempt to make this change other places in the plugin as well, but I'm not very familiar with the code base yet.

[jonknapp]

Thought about it a bit more and the reason for sourceDir is probably to support VS Code opening an individual file instead of a folder or workspace. That would align with why it was second in precedence before the workspace changes.

[wingrunr21]

We support variable interpolation of the path: ${workspaceRoot} gets replaced with the actual path to the workspace root in the configuration.

Multi-root workspace support wasn't available in VSCode when that change went in (it shipped shortly thereafter). This whole extension needs multi-root support but I'm baking that into the language server work. The monorepo support in this extension is pretty bad but is something I'm actively working to address in the next language server update.

[jonknapp]

There may be a disconnect about the part that's incorrect so I'll try to outline it in a bit more detail.

If I wanted to run the rubocop command from the command line it would look similar to the following with the way args are set in the plugin:

cd project_folder
cat db/migrate/000000000_file.rb | bundle exec rubocop -s /Users/jon/project_folder/db/migrate/000000000_file.rb -f json --force-exclusion

To run the same command in node, we'd spawn a process with cwd equal to /Users/jon/project_folder and then pass the arguments that we do today. The difference is that today the cwd is being set to the folder that contains the file that is being linted by stdin which looks like:

cd project_folder/db/migrate
cat 000000000_file.rb | bundle exec rubocop -s /Users/jon/project_folder/db/migrate/000000000_file.rb -f json --force-exclusion

Rubocop uses the correct file to attribute the errors to, but since the command is ran from a different working directory it is interpreting the exclusion paths incorrectly. That means the linter will return errors that we'd normally never see if we used the --force-exclusion flag from the command line or by setting cwd to the project root.

If multi-workspace support is coming and monorepo support is not the best today, could an option be to fix this now and better support monorepos in the future?

Is there anything I can do to assist in fixing this?

[wingrunr21]

There's no disconnect. I understand that the directories being set in exclude have incorrect paths upon execution.

There are two problems with this PR:

1. rootPath is never going to be falsey so there is really no point in keeping sourceDir. To be honest, the same can probably be said about the existing logic though.
2. By running from the project root you cannot take into account .rubocop.yml or Gemfiles that exist deeper in the project directory structure. This could be solved by multi-root, yes, but off the top of my head people with rubygems vendored into lib may not want to be running a multi-root workspace just so their different RuboCop config gets properly picked up.

What I think the correct fix for this is that paths set for the RuboCop settings should be resolved to be absolute paths such that it doesn't matter what working directory RuboCop is run in.

[jonknapp]

Thanks for continuing to consider the conversation.

In response to 1, I agree. However, I figured switching things back to the way they used to be was a less intrusive change than trying to refactor out code that may no longer be accessible. I'm not sure how linters handle unsaved, in-memory only files in VS Code for example.

For number 2, the answer is a bit more complicated. Nested .rubocop.yml files should not be a problem for the scenario you outlined. When rubocop runs, it adapts its rules to nested config files if they are found. (see https://github.com/rubocop-hq/rubocop/blob/master/manual/configuration.md#include-and-exclude-are-relative-to-their-directory for more info) The linter command is still ran relative to the root of the project (normally where the Gemfile is located) and the configuration used per file is adapted by the linter for nested subdirectory configs automatically.

In regards to nested Gemfiles, that does not appear to be something that Bundler supports (rubygems/bundler-features#65) and for projects desiring this functionality, I believe VS Code multi-root workspaces are the way to go. While the current linter code may appear to solve some of their issues, they are still effected by any configuration dependent on relative paths.

A potential path forward would be using the linted files directory and traversing up to the closest Gemfile to run the command from that directly. However, I feel like that's the wrong direction and only makes the plugin work harder than it should, especially in light of mutli-root functionality.

[wingrunr21]

Bundler supports nested Gemfiles just fine: just not in the explicit relationship you specified. If you run bundle exec from the root of the project then Gemfiles nested within the project will not be taken into account.

Look, I'm going to make this simple: This PR breaks #188's compatibility. Without something to maintain that, I'm not merging it. I understand your difficulty on this but I'm not swapping compatibility with your use case and breaking someone else's.

[wingrunr21]

This shouldn't be needed any longer with #405 merged