Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Standard should use the --no-fix argument when linting #447

Closed
wadetandy opened this issue Mar 5, 2019 · 16 comments
Closed

Standard should use the --no-fix argument when linting #447

wadetandy opened this issue Mar 5, 2019 · 16 comments

Comments

@wadetandy
Copy link
Contributor

Your environment

  • vscode-ruby version: 0.22.2
  • Ruby version: MRI 2.5.1
  • Ruby version manager (if any): rvm
  • VS Code version: 1.31.1
  • Operating System: OSX
  • Using language server? Yes

Expected behavior

When using vscode-ruby with standard rb and the following extension settings:

{
  "ruby.format": "standard",
  "ruby.lint": {
    "standard": true
  },
  "ruby.useLanguageServer": true,
}

I would expect updates to the file to run and show linter errors if present.

Actual behavior

When running against files in this project (I haven't verified with all of them, but any I've tried), the Ruby Language Server sends the following output when opening or changing a file. The below example is specifically for this file.

Lint: executing standardrb -s <project_root>/lib/graphiti/railtie.rb -f json...
Lint: Received invalid JSON from standardrb:

{"metadata":{"rubocop_version":"0.65.0","ruby_engine":"ruby","ruby_version":"2.5.1","ruby_patchlevel":"57","ruby_platform":"x86_64-darwin17"},"files":[{"path":"lib/graphiti/railtie.rb","offenses":[]}],"summary":{"offense_count":0,"target_file_count":1,"inspected_file_count":1}}====================
module Graphiti
  class Railtie < ::Rails::Railtie
    rake_tasks do
      path = File.expand_path(__dir__)
... rest of file contents for the current file ...
@wingrunr21
Copy link
Collaborator

Dup of #435

@wadetandy
Copy link
Contributor Author

Ahh sorry I found that issue but the top didn't match. Didn't see the additional things at the bottom. Is there a workaround in the meantime? If I disable the language server, I don't get any linting/autoformatting at all.

@wingrunr21
Copy link
Collaborator

Ya, I updated the issue title to include Standard as well.

Long story short is that the node spawn is chunking the stdout back into the process. I need to implement a buffer to collect all of this output and process the results at the conclusion of the spawn.

The workaround is to reduce the linter output so that it fits within one chunk. If you fix some of the problems in the file you should start getting linter results back. I realize this is not great but it's the best I've got right now. This is my top bug I'm looking at.

@wadetandy
Copy link
Contributor Author

wadetandy commented Mar 6, 2019

@wingrunr21 Now that #435 is merged, it seems like this is still an issue and an unrelated root cause. As you can see from my console output above, it seems that the actual code that is being linted is getting apppended to the JSON payload, which is resulting in invalid json. Have confirmed by logging data from here and I see:

{"valid json": "from standard"}=======
the ruby code

@wingrunr21
Copy link
Collaborator

wingrunr21 commented Mar 6, 2019

What version of standard?

Also, if you run that exact command in a terminal do you still see the ruby code? I'm not doing any kind of code appending.

@wingrunr21 wingrunr21 reopened this Mar 6, 2019
@wadetandy
Copy link
Contributor Author

I'm using the latest 0.0.36. Interestingly enough it's hanging when I run the same command in my terminal:

bundle exec standardrb -s <the_file> -f json

If I modify your extension to instead run standardrb -f json <the_file>, everything behaves as expected and the linter works.

@wingrunr21
Copy link
Collaborator

wingrunr21 commented Mar 6, 2019

-s is stdin. You have to cat the file and then pipe it to standard

@wadetandy
Copy link
Contributor Author

Oh, just realizing -s is the stdin, which is why it's hanging. When I pass the file via stdin, it comes back in the output after the json. I'll open an issue with standard.

@wingrunr21
Copy link
Collaborator

So I do not get that behavior. Do you have any additional configuration set up for standard?

@wadetandy
Copy link
Contributor Author

wadetandy commented Mar 6, 2019

Ok, after trying to isolate it to create an issue with standard, I've figured out the problem. My project keeps the Standard configuration using a .standard.yml file. In that file, I have the fix: true option set for autofixing issues. If you either set that option or pass --fix to either rubocop or standard, the response will be:

{"valid json": "from standard"}=======
fixed ruby code

Seems that the language server can either split on the ======== and discard anything after it (or take advantage of it, potentially) or explicitly pass the --no-fix cli option, which will override the setting in the config file.

@wingrunr21
Copy link
Collaborator

Well, the --no-fix option is definitely the right way to solve this vs splitting on the output. The tradeoff here is that a user would then need to perform a format in order to get the fixes in place.

This would end up being an unconfigurable override to the config file. What are your thoughts on that?

@wadetandy
Copy link
Contributor Author

Yeah, splitting the output would be useful if you were able to apply the autofixed code back to the buffer, but otherwise wouldn't be helpful and the no-fix would be correct. My understanding is that we already don't get autofixes applied unless we run the formatter on save, so we're no worse off here, correct?

@wingrunr21
Copy link
Collaborator

I could split the output (as that's how the formatter works) but there's no way to push that back to the editor. Linter diagnostics are pushed from the server to the client but text edit requests are round trip originating from the client. It'd also be a little weird since linting happens on text change by default. The whole document could be shifting under your cursor.

Well, you can turn on format on save in the editor. That'd achieve the same purpose. I haven't implemented format as you type yet because rubocop/standard/etc are too slow. rubyfmt may end up being fast enough to support that.

Sounds like applying --no-fix is the answer. That should be a pretty quick change.

@wingrunr21 wingrunr21 reopened this Mar 7, 2019
@wingrunr21 wingrunr21 changed the title StandardRB fails with language server Standard should use the --no-fix argument when linting Mar 7, 2019
@wadetandy
Copy link
Contributor Author

wadetandy commented Mar 7, 2019

Thanks! Any way you can cut a new release soon?

@wingrunr21
Copy link
Collaborator

The next release will be a minor version due to a ruby-method-locate upgrade so there are a few other things I want to get in.

@wadetandy
Copy link
Contributor Author

ah no worries then, take your time!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants