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

feat: add non-matching output lint rule #114

Merged
merged 35 commits into from
Jul 16, 2024
Merged

Conversation

adthrasher
Copy link
Member

This pull request adds a new rule to wdl-lint.

  • Rule Name: NonmatchingOutput

This lint rule ensures that if output is populated, then there is an outputs key in the meta section. It also ensures that each output has a corresponding entry in outputs.

Before submitting this PR, please make sure:

  • You have added a few sentences describing the PR here.
  • You have added yourself or the appropriate individual as the assignee.
  • You have added at least one relevant code reviewer to the PR.
  • Your code builds clean without any errors or warnings.
  • You have added an entry to the relevant CHANGELOG.md (see
    ["keep a changelog"] for more information).
  • Your commit messages follow the [conventional commit] style.

Rule specific checks:

  • You have added the rule as an entry within RULES.md.
  • You have added the rule to the rules() function in wdl-lint/src/lib.rs.
  • You have added a test cases in wdl-lint/tests/lints that covers every
    possible diagnostic emitted for the rule within the file where the rule
    is implemented.
  • You have run wdl-gauntlet --refresh to ensure that there are no
    unintended changes to the baseline configuration file (Gauntlet.toml).
  • You have run wdl-gauntlet --refresh --arena to ensure that all of the
    rules added/removed are now reflected in the baseline configuration file
    (Arena.toml).

@a-frantz
Copy link
Member

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 output without the 's'.

@adthrasher adthrasher self-assigned this Jul 10, 2024
@adthrasher adthrasher marked this pull request as ready for review July 10, 2024 14:40
@adthrasher
Copy link
Member Author

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 output without the 's'.

@a-frantz - That's a good question. We were inconsistent in the workflows repo. I suppose output isn't a keyword when in a meta section.

wdl-lint/src/rules/nonmatching_output.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/nonmatching_output.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/nonmatching_output.rs Outdated Show resolved Hide resolved
match reason {
VisitReason::Enter => {
self.name = Some(workflow.name().as_str().to_string());
self.ty = Some("workflow");
Copy link
Member

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?

Copy link
Member Author

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.

adthrasher and others added 2 commits July 15, 2024 09:59
Co-authored-by: Andrew Frantz <andrew.frantz@stjude.org>
@claymcleod claymcleod self-requested a review July 15, 2024 15:03
@adthrasher adthrasher requested a review from a-frantz July 15, 2024 20:01
check_matching(state, self);
}

self.name = None;
Copy link
Member

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 => {}
Copy link
Member

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.

Copy link
Member Author

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 {
Copy link
Member

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() {
Copy link
Member

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.

wdl-lint/src/lib.rs Outdated Show resolved Hide resolved
@adthrasher adthrasher merged commit d45f7f2 into main Jul 16, 2024
7 checks passed
@adthrasher adthrasher deleted the feat/nonmatching_output branch July 16, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants