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

rewrite or remove existing fix messages #193

Open
a-frantz opened this issue Sep 26, 2024 · 1 comment
Open

rewrite or remove existing fix messages #193

a-frantz opened this issue Sep 26, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@a-frantz
Copy link
Member

a-frantz commented Sep 26, 2024

Many of our fix messages are redundant, obvious, or otherwise unhelpful. We should use the fix feature more mindfully. That means supplying the "corrected" code or adding something to the diagnostic that isn't already conveyed. And if those can't be accomplished, the fix should be omitted.

related to #192

@a-frantz
Copy link
Member Author

Copying some thoughts I typed up in slack:

Existing diagnostic:

note[BlankLinesBetweenElements]: missing blank line
   ┌─ tests/lints/blank-lines-between-elements/source.wdl:17:5
   │  
17 │ ╭     scatter (i in ["hello", "world"]) {
18 │ │         call bar { input: s = i }
19 │ │     }
   │ ╰─────^
   │  
   = fix: add a blank line before this element

We discussed being more intentional with the fix messages. So we probably want to drop this one as it doesn't add very much. That leaves:

note[BlankLinesBetweenElements]: missing blank line
   ┌─ tests/lints/blank-lines-between-elements/source.wdl:17:5
   │  
17 │ ╭     scatter (i in ["hello", "world"]) {
18 │ │         call bar { input: s = i }
19 │ │     }
   │ ╰─────^

Problem here is that now the Span doesn't make sense. Why are lines 18-19 included? Maybe the span's len should be 0? That creates:

note[BlankLinesBetweenElements]: missing blank line
   ┌─ tests/lints/blank-lines-between-elements/source.wdl:17:5
   │
17 │     scatter (i in ["hello", "world"]) {
   │     ^

Ideally, I want an arrow that points between lines 16 and 17 and indicates a blank goes here. IDK how to do that in codespan. Can we do that?

Created by hand, but something like:

   ┌─ tests/lints/blank-lines-between-elements/source.wdl:17:5
16 │     input {}
---->
17 │     scatter (i in ["hello", "world"]) {

(We can't do that in codespan)

The moral of this experiment is that reworking the fix messages may also require changing up some of the reported Spans to make more sense. That can be a bit tricky depending on how the rule is written. So just something to be mindful of.

@a-frantz a-frantz added the enhancement New feature or request label Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant