-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: add non-matching output lint rule #114
Conversation
…ust-labs/wdl into feat/nonmatching_output
Maybe an annoying question to ask: does it make more sense to have the key named the same as the section? i.e. both of them |
@a-frantz - That's a good question. We were inconsistent in the |
match reason { | ||
VisitReason::Enter => { | ||
self.name = Some(workflow.name().as_str().to_string()); | ||
self.ty = Some("workflow"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we follow the example of other lint rules and wrap this in an enum Context
instead of passing around str
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably could, but that seems a heavy solution to ultimately print task
or workflow
. Before going that route, I'd actually prefer if we could store a TaskOrWorkflow
instance and use that instead, rather than creating a new enum
to handle it.
Co-authored-by: Andrew Frantz <andrew.frantz@stjude.org>
check_matching(state, self); | ||
} | ||
|
||
self.name = None; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is common with the code in the task_definition
callback, I might pull it out into its own function.
VisitReason::Enter => { | ||
self.current_meta_span = Some(section.syntax().text_range().to_span()); | ||
} | ||
VisitReason::Exit => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be explicit and avoid future issues, would this be a place to set self.current_meta_span = None
? It's probably inconsequential unless something is terribly wrong, but it's worth considering being explicit.
If the answer above is "no", then I feel this should at least be an if statement like in output_section
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, it would require a refactor of the Exit
logic for both Task
and Workflow
.
reason: VisitReason, | ||
section: &OutputSection, | ||
) { | ||
if reason == VisitReason::Enter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same consideration here.
if reason == VisitReason::Enter { | ||
if let Some(output) = &self.current_output_span { | ||
let decl_span = decl.syntax().text_range().to_span(); | ||
if decl_span.start() > output.start() && decl_span.end() < output.end() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's one way to check if the decl is within the output 😄.
Based on my last comment, you might could check if self.current_output_span
is None
if you took that approach.
This pull request adds a new rule to
wdl-lint
.NonmatchingOutput
This lint rule ensures that if
output
is populated, then there is anoutputs
key in themeta
section. It also ensures that eachoutput
has a corresponding entry inoutputs
.Before submitting this PR, please make sure:
CHANGELOG.md
(see["keep a changelog"] for more information).
Rule specific checks:
RULES.md
.rules()
function inwdl-lint/src/lib.rs
.wdl-lint/tests/lints
that covers everypossible diagnostic emitted for the rule within the file where the rule
is implemented.
wdl-gauntlet --refresh
to ensure that there are nounintended changes to the baseline configuration file (
Gauntlet.toml
).wdl-gauntlet --refresh --arena
to ensure that all of therules added/removed are now reflected in the baseline configuration file
(
Arena.toml
).