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

Add option to run external linter on each save #5542

Open
Kobzol opened this issue Jun 9, 2020 · 3 comments
Open

Add option to run external linter on each save #5542

Kobzol opened this issue Jun 9, 2020 · 3 comments
Labels
improvement subsystem::code insight General label for issues related to code understanding: highlighting, completion, annotation, etc. subsystem::tools Issues about integration with external tools like rustup, cargo, rustfmt, etc.

Comments

@Kobzol
Copy link
Member

Kobzol commented Jun 9, 2020

Right now there are two options of running an external linter:

  1. Manually using the Run external linter action
  2. Automatically using the Run external linter to analyze code on the fly option

The first option works fine, but it requires manual user action to invoke the linter and it doesn't show the results in the code editor.
The second option runs the linter automatically, but it can be quite slow, particularly on larger projects (from my experience, and also mentioned by @teivah on gitter) and the timing feels a bit weird sometimes (there's a 300ms delay I think).

Could we add an option to run the external linter on each save, similarly to the option of running rustfmt on each save? It would be deterministic for the user (invoked after a save), it wouldn't need to run periodically, so it would save battery/performance and also it would show up in the editor (I think that currently the only option to show linter errors directly in the editor is to use the "on the fly" option, which has the mentioned disadvantages).

It could either replace the "on-the-fly" mode or it could be another option (run linter: never/on-the-fly/on-save).

@Kobzol Kobzol added the subsystem::tools Issues about integration with external tools like rustup, cargo, rustfmt, etc. label Jun 9, 2020
@panstromek
Copy link
Contributor

panstromek commented Jun 9, 2020

I opened a similar issue a while ago - #4779

It's similar but a bit different. Basically I'd like to have the 1. option (and bind it to a shortcut), but seeing the results in the editor, same as with the 2. option.

Having it run on each save would definitely be nice for bigger projects, but it would still be problematic to use with hot reloading (like with webpack and wasm-pack, as mentioned in the other issue), because once you save the file, wasm-pack and linter will both fight over target dir lock, blocking each other from completion.

I tried to look at the code for the annotator on the weekend, to try if I could find a way to do this, but I haven't fully understood how all of it works, yet.

@Kobzol Kobzol added improvement subsystem::code insight General label for issues related to code understanding: highlighting, completion, annotation, etc. labels Jun 10, 2020
@Kobzol
Copy link
Member Author

Kobzol commented Jun 10, 2020

So to sum up, there are (at least) three ways how to run the external linter:

  1. (Key-bindable) action
  2. On save
  3. Periodically ("on-the-fly")

And there are (at least) two ways of displaying the results:
A) In editor
B) In inspection results window/tab

Currently the plugin supports 1+B and 3+A. @teivah would prefer 2+A and @panstromek would prefer 1+A. In addition to supporting running the linter on save, it seems that we could add an option that would control whether to display the results in the editor or in the inspection window (or both). Although I'm not sure for how long should the results exist in the editor if they are invoked via a keybind/save (when will they be invalidated)?

@mchernyavsky what do you think? :)

@Kobzol Kobzol changed the title Add option to add external linter on each save Add option to run external linter on each save Jun 25, 2020
@whatisaphone
Copy link

I'm also using wasm-pack. So like others, I can't use "on the fly" analysis, because of constant webpack rebuilds.

For me, setting org.rust.cargo.build.tool.window = true is already close to ideal. That sets behavior for the Build action. Auto-navigating to the first error is particularly helpful! Two minor nits:

  • It doesn't highlight error spans in the editor
  • It uses cargo test --no-run, while I think it would make more sense to use cargo check/clippy as set in the settings. (This is the only reason I'm here looking at issues in the first place)

So I'd add the org.rust.cargo.build.tool.window workflow to @Kobzol's lists:

1. (Key-bindable) action
2. On save
3. Periodically ("on-the-fly")
4. Platform Build action

A) Highlight spans in the editor
B) List errors in inspection results window/tab
C) List errors in build results window
D) auto-navigate to first error

My small feature request is changing the tool window to use clippy if clippy is enabled in the settings. Or bigger picture, maybe it makes sense to merge what seems like separate code paths in the plugin, and take the best parts of each.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement subsystem::code insight General label for issues related to code understanding: highlighting, completion, annotation, etc. subsystem::tools Issues about integration with external tools like rustup, cargo, rustfmt, etc.
Projects
None yet
Development

No branches or pull requests

3 participants