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: document_preamble lint rule #25

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

adthrasher
Copy link
Member

@adthrasher adthrasher commented May 2, 2024

This pull request adds a new rule to wdl.

  • Rule Name: document_preamble
  • Rule Kind: Lint warning
  • Rule Code: v1::W009
  • Packages: wdl-grammar

Implements document_preamble as defined in #11.

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.
  • Your changes are squashed into a single commit (unless there is a really
    good, articulated reason as to why there shouldn be more than one).

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.
  • You have run wdl-gauntlet --save-config to ensure that all of the rules
    added/removed are now reflected in the baseline configuration file
    (Gauntlet.toml).

@adthrasher adthrasher requested a review from claymcleod May 2, 2024 18:36
@adthrasher adthrasher self-assigned this May 2, 2024
break;
}
_ => {
anything_else += 1;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be unreachable!()? When could something other than COMMENT or WHITESPACE be found before version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so, so I changed it. However, it broke a bunch of other tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like the lints are being run as part of some of the other tests, e.g.

fn it_parses_from_a_supported_node_type() {
. Since those are testing individual nodes, there is no version declaration. I'm not sure if the lints should be running, I would have thought not. Maybe @claymcleod can weigh in.

Copy link
Member

@claymcleod claymcleod May 3, 2024

Choose a reason for hiding this comment

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

I think, for individual tests, the lints can be disabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not clear how they're being run (or how to disable, if that's what should happen). It's when I run cargo test --all-features, but it produces a long list of failures, most of which are it_parses_from_a_supported_node_type type tests.

    v1::document::declaration::bound::tests::it_parses_from_a_supported_node_type
    v1::document::declaration::unbound::tests::it_parses_from_a_supported_node_type
    v1::document::expression::array::tests::it_parses_from_a_supported_node_type
    v1::document::expression::map::tests::it_parses_from_a_supported_node_type
    v1::document::expression::pair::tests::it_parses_from_a_supported_node_type
    v1::document::expression::r#if::tests::it_parses_from_a_supported_node_type
    v1::document::expression::tests::ensure_number_works_correctly
    v1::document::expression::tests::it_correctly_parses_floats
    v1::document::expression::tests::it_ignores_comments_and_whitespace
    v1::document::identifier::qualified::tests::it_parses_from_a_supported_node_type
    v1::document::identifier::tests::it_parses_from_a_supported_node_type
    v1::document::import::tests::it_parses_a_complicated_import_correctly
    v1::document::output::builder::tests::it_parses_from_a_supported_node_type
    v1::document::private_declarations::tests::it_parses_from_a_supported_node_type
    v1::document::r#struct::tests::it_parses_from_a_supported_node_type
    v1::document::task::runtime::value::tests::it_correctly_parses_floats
    v1::document::task::runtime::value::tests::it_correctly_parses_integers
    v1::document::tests::it_parses_from_a_supported_node_type
    v1::document::workflow::execution::statement::call::body::tests::it_parses_from_a_supported_node_type
    v1::document::workflow::execution::statement::call::tests::it_parses_from_a_supported_node_type
    v1::document::workflow::execution::statement::conditional::tests::it_parses_from_a_supported_node_type
    v1::document::workflow::execution::statement::scatter::tests::it_parses_from_a_supported_node_type

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. It's here in parse_rule.

wdl/wdl-grammar/src/v1.rs

Lines 177 to 179 in 35731ec

if let Some(warnings) = Linter::lint(&pt, lint::rules()).map_err(Error::Lint)? {
for warning in warnings {
concerns = concerns.push(Concern::LintWarning(warning));

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see a way to disable running the lint rules in that function. It does appear that nothing is done with any lint warnings. Was this intentional?

@adthrasher adthrasher changed the title feat: version_declaration_placement lint rule feat: document_preamble lint rule May 3, 2024
@a-frantz a-frantz mentioned this pull request May 9, 2024
11 tasks
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.

None yet

3 participants