stjude-rust-labs / wdl

Rust crates for working with Workflow Description Language (WDL) documents.
Apache License 2.0
24 stars 6 forks source link

rewrite or remove existing `fix` messages #193

Open a-frantz opened 3 days ago

a-frantz commented 3 days ago

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 commented 2 days ago

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.