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: adds check for tasks with no runtime block #10

Merged
merged 15 commits into from
Apr 11, 2024

Conversation

markjschreiber
Copy link
Contributor

@markjschreiber markjschreiber commented Apr 11, 2024

This pull request adds a new rule to wdl.

  • Rule Name: missing_runtime_block
  • Rule Kind: Lint warning
  • Rule Code: v1::W005
  • Packages: wdl-grammar

Describe the rules you have implemented and link to any relevant issues.

The rule will warn if a task doesn't contain a runtime{} block because this is likely to limit the portability of the task.

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.

Rule specific checks:

  • You have added the rule as an entry within the the package-specific rule
    tables (wdl-ast/src/v1.rs for AST-based rules and
    wdl-grammar/src/v1.rs for parse tree-based rules).
  • You have added the rule as an entry within the the global rule
    table at RULES.md.
  • You have added the rule to the appropriate fn rules().
    • Validation rules added to wdl-ast should be added to fn rules() within
      wdl-ast/src/v1/validation.rs.
    • Lint rules added to wdl-ast should be added to fn rules() within wdl-ast/src/v1/lint.rs.
    • Validation rules added to wdl-grammar should be added to fn rules() within
      wdl-grammar/src/v1/validation.rs.
    • Lint rules added to wdl-grammar should be added to fn rules() within
      wdl-grammar/src/v1/lint.rs.
  • You have added a test that covers every possible setting for the rule
    within the file where the rule is implemented.

@claymcleod claymcleod self-requested a review April 11, 2024 16:32
@claymcleod claymcleod added the enhancement New feature or request label Apr 11, 2024
Copy link
Member

@claymcleod claymcleod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some thoughts on the code and a few more thoughts outside of that.

  • We discussed it internally, and we good with this lint being standalone. There is also the concept that some of the runtime keys should be specified (for instance, container is almost certainly required to be portable). Rather than pushing for that to be included in this lint, we propose that we make a separate lint later on that addresses these—that way, users can configure separately whether they want to check for runtime blocks (almost always) versus what particular keys they want to enforce checking.
  • Sadly, I think runtime is going away in WDL 1.2. So it's just worth noting that, when we do end up updating to 1.2, this lint may actually need to change to a deprecation warning for runtime and a new one will need to take it's place for the new keys.
  • If you would, go ahead and squash down your changes into one commit so I can see what the final commit added to the history will be. This will require a force push to your branch.

RULES.md Outdated Show resolved Hide resolved
wdl-grammar/src/v1.rs Show resolved Hide resolved
wdl-grammar/src/v1/lint/missing_runtime_block.rs Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/missing_runtime_block.rs Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/missing_runtime_block.rs Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/missing_runtime_block.rs Outdated Show resolved Hide resolved
@claymcleod
Copy link
Member

Also, I will just note here for myself: we need to add a CHANGELOG.md and guidelines for adding entries to the CHANGELOG.md when these PRs are created.

markjschreiber and others added 5 commits April 11, 2024 14:22
Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
@markjschreiber
Copy link
Contributor Author

Here are some thoughts on the code and a few more thoughts outside of that.

  • We discussed it internally, and we good with this lint being standalone. There is also the concept that some of the runtime keys should be specified (for instance, container is almost certainly required to be portable). Rather than pushing for that to be included in this lint, we propose that we make a separate lint later on that addresses these—that way, users can configure separately whether they want to check for runtime blocks (almost always) versus what particular keys they want to enforce checking.

Agreed, more granular lint checks help with configurable checking and can give better messages and fix suggestions

  • Sadly, I think runtime is going away in WDL 1.2. So it's just worth noting that, when we do end up updating to 1.2, this lint may actually need to change to a deprecation warning for runtime and a new one will need to take it's place for the new keys.

Yes, it will be deprecated and replaced with requirements so the content of this check will essentially move to a requirements check.

  • If you would, go ahead and squash down your changes into one commit so I can see what the final commit added to the history will be. This will require a force push to your branch.

Will do!

@claymcleod claymcleod merged commit 44c908e into stjude-rust-labs:main Apr 11, 2024
6 checks passed
claymcleod added a commit that referenced this pull request Apr 11, 2024
This feature had the following commit messages that were squashed.

* adds toolchain.toml for nightly channel
* adds test for tasks with no runtime block
* revise: updates rule number
* feat: adds missing_runtime_block rule
* revise: applies cargo fmt
* revise: fixes doc test after addition of new rule
* revise: adds new concerns for missing runtime block
* Update RULES.md
* Update wdl-grammar/src/v1/lint/missing_runtime_block.rs
* revise: adjusts warning message

Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
claymcleod added a commit that referenced this pull request Apr 11, 2024
This feature had the following commit messages that were squashed.

* adds toolchain.toml for nightly channel
* adds test for tasks with no runtime block
* revise: updates rule number
* feat: adds missing_runtime_block rule
* revise: applies cargo fmt
* revise: fixes doc test after addition of new rule
* revise: adds new concerns for missing runtime block
* Update RULES.md
* Update wdl-grammar/src/v1/lint/missing_runtime_block.rs
* revise: adjusts warning message

Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
claymcleod added a commit that referenced this pull request Apr 11, 2024
This feature had the following commit messages that were squashed.

* adds toolchain.toml for nightly channel
* adds test for tasks with no runtime block
* revise: updates rule number
* feat: adds missing_runtime_block rule
* revise: applies cargo fmt
* revise: fixes doc test after addition of new rule
* revise: adds new concerns for missing runtime block
* Update RULES.md
* Update wdl-grammar/src/v1/lint/missing_runtime_block.rs
* revise: adjusts warning message

Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
claymcleod added a commit that referenced this pull request Apr 11, 2024
claymcleod added a commit that referenced this pull request Apr 11, 2024
claymcleod added a commit that referenced this pull request Apr 11, 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

Successfully merging this pull request may close these issues.

3 participants