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: adding preamble_comment lint rule #32

Merged
merged 7 commits into from May 13, 2024

Conversation

simojoe
Copy link
Contributor

@simojoe simojoe commented May 9, 2024

This pull request adds a new rule to wdl.

  • Rule Name: preamble_comment
  • Rule Kind: Lint warning
  • Rule Code: v1::W010
  • Packages: wdl-grammar

Verify that comments before (and only before) the version declaration start with a double pound sign.

The whitespace around the pound signs and the comment lines are checked in other rules.

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.
  • [X 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).

Copy link
Member

@a-frantz a-frantz left a comment

Choose a reason for hiding this comment

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

@adthrasher please weigh in, but I'm thinking everywhere this PR says header should be updated to preamble so that we're consistent with the language adopted in #25 .

I coined the term "header comment" but I think before solidifying that in code we should pivot to calling them "preamble comments". Does that make sense?

wdl-grammar/src/v1/lint/header_comment.rs Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/header_comment.rs Outdated Show resolved Hide resolved
@adthrasher
Copy link
Member

@adthrasher please weigh in, but I'm thinking everywhere this PR says header should be updated to preamble so that we're consistent with the language adopted in #25 .

I coined the term "header comment" but I think before solidifying that in code we should pivot to calling them "preamble comments". Does that make sense?

I think we need to be careful here. For example in this text, double pound signs are reserved for header comments, the double pound signs would be a preamble comment, but they're reserved for use in the document preamble. I would reword it to something like double pound signs/preamble comments are reserved for the document preamble.

@a-frantz
Copy link
Member

I think we need to be careful here. For example in this text, double pound signs are reserved for header comments, the double pound signs would be a preamble comment, but they're reserved for use in the document preamble. I would reword it to something like double pound signs/preamble comments are reserved for the document preamble.

Good catch that it will be more than a simple find and replace. But sounds like you're on board with the terminology change?

Going forward, we're going to retire the term "header" and instead use "preamble".

Besides some word smithing related to this change, the PR looks good to me. I requested @peterhuene as a reviewer to make sure I didn't miss anything on the Rust side of things.

Copy link
Collaborator

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

From the Rust side of things, looks good 👍

@adthrasher
Copy link
Member

I think we need to be careful here. For example in this text, double pound signs are reserved for header comments, the double pound signs would be a preamble comment, but they're reserved for use in the document preamble. I would reword it to something like double pound signs/preamble comments are reserved for the document preamble.

Good catch that it will be more than a simple find and replace. But sounds like you're on board with the terminology change?

Going forward, we're going to retire the term "header" and instead use "preamble".

Besides some word smithing related to this change, the PR looks good to me. I requested @peterhuene as a reviewer to make sure I didn't miss anything on the Rust side of things.

Yes, I think that's a better approach moving forward.

@simojoe simojoe requested a review from a-frantz May 11, 2024 23:20
@simojoe
Copy link
Contributor Author

simojoe commented May 11, 2024

I updated the naming as I understood from the conversation above!

I kept the condition disallowing the normal comments in the preamble as was not clear if the converse was decided.

@adthrasher adthrasher changed the title feat: adding header_comment lint rule feat: adding preamble_comment lint rule May 13, 2024
Copy link
Member

@a-frantz a-frantz left a comment

Choose a reason for hiding this comment

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

@adthrasher this looks good to me. We can go ahead and merge if you give the all clear.

Thanks a lot @simojoe ! We appreciate your contributions!

@adthrasher adthrasher merged commit d009534 into stjude-rust-labs:main May 13, 2024
6 checks passed
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

4 participants