Add sprocket lint CI job - #178
Conversation
These are accepted by `miniwdl check` but rejected as errors by the WDL specification, so they blocked introducing any stricter analysis in CI. `\|` and `\(` are not valid WDL escape sequences. The `DECOY_REGEX` defaults meant to pass a literal `\|` through to `grep`, so they now use `\\|`, matching the already-correct definition in `vg_trio_giraffe_deeptrio_workflow.wdl`. `read_map` yields `Map[String, String]`, which cannot satisfy the `Map[String, Int]` output of `runMakeContigMAP`. The task now emits JSON and reads it with `read_json`, preserving the integer indices that `eagle_vcf_contig_index` consumes. `output` is a reserved keyword and cannot name a declaration, so `falseTouchFile` now returns `output_file`. Nothing imports `generic_tasks.wdl`, so the rename is contained. `vg_map_hts` requires a non-optional `File`, but `vg_construct_and_index` exports `gcsa` and `gcsa_lcp` as `File?`, so the SV test now unwraps them with `select_first`.
The rule selection follows `stjudecloud/workflows`, the reference WDL repo from the same authors as `sprocket`: `all_lint_rules` on, `deny_notes` on, and `ContainerUri`, `TodoComment`, and `UnusedInput` excepted. `InputName`, `OutputName`, and `SnakeCase` are excepted on top of that because they conflict with conventions this repo already applies consistently rather than with mistakes. Task inputs carry an `in_` prefix in 995 of 1005 findings, workflow inputs are SCREAMING_SNAKE_CASE across 220 distinct names, and task names are camelCase in 166 of 184 cases. Enforcing the WDL standard names would rewrite the public interface of every workflow without fixing anything. See vgteam#12. The remaining 1867 findings go in `sprocket-baseline.toml` so only new ones fail the build. Entries match on a hash of the flagged source, which means editing a previously flagged line makes its entry stale and fails the build until the baseline is regenerated; the job explains this on failure.
The rule shells out to whatever `shellcheck` is on `PATH`. Version 0.10 on `ubuntu-latest` reports `SC2236` and `SC2002` where 0.11 does not, so CI found 11 diagnostics absent from a baseline generated locally. Generating it on the runner instead would invert the problem, since the baseline fails on stale entries as well as new ones. `miniwdl check` already runs shellcheck over command sections, so no coverage is lost. This also drops the baseline from 1867 entries to 724, all from rules that depend only on the WDL source.
d2f7205 to
4b01017
Compare
adamnovak
left a comment
There was a problem hiding this comment.
I like having a Sprocket-based lint step, and the fixes to the WDL code.
I don't like the idea of carrying a file full of 724 hashes of code lines to exempt from particular linter opinions. This is for a few reasons:
- I would like it to be possible for someone who does not themselves have a working
sprocketinstall of exactly version0.28.0to produce a PR and address any lint failures from CI with just their text editor. If some CI failures need to be addressed by computing a hash and adding it to a file, or regenerating the baseline to remove no-longer-used exceptions, that's not possible. - It's hard for me to believe that, in exactly all 724 of these cases, we don't want to bother to follow the lint rule, but in future similar code, we do genuinely intend to follow that rule. We might want to make any rule that has more than a handful of individual exemptions just globally exempted, until we have the attention to shift the code overall to actually comply with that rule.
- I think the large number of items reflect the fact that the linter is set to fail at
noteseverity, and, at least at that level, has a number of opinions that might really be Sprocket team conventions rather than WDL language conventions. There's anUnusedDocCommentthat goes off whenever a##is somewhere that St. Jude'swdl-doctool can't use to generate documentation, for example, but the notion of doc comments isn't in the WDL spec even as of 1.4. It might make more sense to raise the severity level we fail on to things that Sprocket actually can't handle, or which don't look reasonable under anybody's house style, and then lower it as Sprocket-style WDL takes over the world and becomes the new Black.
| # SC2002 where 0.11 does not. Because the baseline fails on stale entries as | ||
| # well as new ones, that drift breaks the build in both directions depending | ||
| # on who generated the baseline. `miniwdl check` already runs shellcheck over | ||
| # command sections, so excepting it here loses no coverage. |
There was a problem hiding this comment.
I'd cut this explanation, because the notion that the linter is very insistent that all exceptions be used doesn't really belong to ShellCheck, and is sort of implied by the fact that we're exempting for nondeterministic behavior.
| # SC2002 where 0.11 does not. Because the baseline fails on stale entries as | |
| # well as new ones, that drift breaks the build in both directions depending | |
| # on who generated the baseline. `miniwdl check` already runs shellcheck over | |
| # command sections, so excepting it here loses no coverage. | |
| # SC2002 where 0.11 does not. |
| # Task inputs are prefixed `in_` (995 of 1005 findings) and workflow-level | ||
| # inputs are SCREAMING_SNAKE_CASE (220 distinct names). Task names are | ||
| # camelCase (166 of 184). These are deliberate and repo-wide, so enforcing | ||
| # the WDL standard names would mean rewriting the public interface of every | ||
| # workflow rather than fixing anything. |
There was a problem hiding this comment.
These numbers will immediately become wrong, and this is more commit message than comment.
| # Task inputs are prefixed `in_` (995 of 1005 findings) and workflow-level | |
| # inputs are SCREAMING_SNAKE_CASE (220 distinct names). Task names are | |
| # camelCase (166 of 184). These are deliberate and repo-wide, so enforcing | |
| # the WDL standard names would mean rewriting the public interface of every | |
| # workflow rather than fixing anything. |
If we want to convey these points, we could comment each exception with the kinds of names we use that it disallows.
|
Yeah this is all fair enough. In truth, this approach only makes sense if you don't want to do the work of implementing all the lints. But I'm happy to reverse it and go that direction. Sprocket will need some configuring which, as you say, is a preference thing. Do you want to do that work or do you want me to make my best guess? Sidenote, in an upcoming PR (stjude-rust-labs/sprocket#963), we're going to greatly simplify and improve the configuration of these rules. So you could also choose to wait (or just be ready to change the configuration in the future). |
The repo had no lint coverage, and
sprocket checkreported 16 hard errors thatminiwdl checkaccepts: invalid\|and\(escape sequences in twoDECOY_REGEXdefaults, aread_mapresult declared asMap[String, Int], a declaration named with the reservedoutputkeyword, and twoFile?values passed whereFilewas required. Those were fixed first, since analysis errors cannot be recorded in a baseline and would have kept the job red. Theread_mapcase now emits JSON and reads it back withread_json, since WDL 1.0 has no string-to-integer coercion.The rule selection follows
stjudecloud/workflows, the reference WDL repo from the same authors assprocket, which enables all lint rules, denies notes, and exceptsContainerUri,TodoComment, andUnusedInput. I exceptedInputName,OutputName, andSnakeCaseon top of that because they conflict with conventions this repo already applies consistently rather than with mistakes: task inputs carry anin_prefix in 995 of 1005 findings, workflow inputs are SCREAMING_SNAKE_CASE across 220 distinct names, and task names are camelCase in 166 of 184 cases. Enforcing the standard names would rewrite the public interface of every workflow without fixing anything. That judgment is the part most worth a second opinion, and it overlaps with the naming question in #12.ShellCheckis excepted for a different reason. The rule shells out to whatevershellcheckis onPATH, and version 0.10 onubuntu-latestreportsSC2236andSC2002where 0.11 does not, so the first CI run failed on 11 diagnostics absent from a baseline generated locally. Generating it on the runner would invert the problem, because the baseline fails on stale entries as well as new ones.miniwdl checkalready runs shellcheck over command sections, so no coverage is lost.The remaining 724 findings live in
sprocket-baseline.tomlso only new ones fail the build. Entries match on a hash of the flagged source, so editing a previously flagged line makes its entry stale and fails the build until someone runssprocket lint --generate-baseline workflows tasks testsand commits the result. The job explains this on failure andCLAUDE.mddocuments it, but it is the main ongoing friction of this setup and worth weighing before merge.I verified the guard rail end to end by pushing a task with a new violation and confirming CI failed on it while correctly ignoring its
in_prefix and camelCase name, then removed it.