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

fix: reworks the concept of lint directives #162

Merged
merged 19 commits into from
Sep 16, 2024
Merged

Conversation

a-frantz
Copy link
Member

@a-frantz a-frantz commented Aug 26, 2024

Describe the problem or feature in addition to a link to the issues.

This PR reworks the concept of lint directives (AKA #@ comments or except comments). It should close #135 , which details how lint directives have been partially broken since their introduction.

Lint directives keep their same form (#@ except: <comma delimited list of Rule IDs>) but now are only expected in certain locations. Those locations are defined on a per-rule basis via the new exceptable_nodes() method in the Rule trait.

TODO in follow-up PR[s]

  • We've discussed the idea of firing diagnostics for lint directives that have no effect (but are in the correct location). This will require some further changes to the Diagnostics struct not included here.
  • IMO the WDL in our tests/lints/ dir is awfully messy. They frequently contain a long list of excepted rules. All those WDL files deserve a clean up to be more conformant to expected lints.
  • Refactor PreambleWhitespace and PreambleComments into new rules PreambleFormatting and PreambleCommentInBody (accepting alternate names there). PreambleFormatting will perform the bulk of the checks currently handled by the whitespace and comments rules. PreambleCommentInBody will check for double-pound-sign comments outside the preamble.

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 tests (when appropriate).
  • You have updated the README or other documentation to account for these
    changes (when appropriate).
  • 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.

@a-frantz a-frantz self-assigned this Aug 26, 2024
@a-frantz a-frantz marked this pull request as ready for review September 11, 2024 20:24
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.

This looks really great! I've added some review comments.

wdl-lint/src/lib.rs Outdated Show resolved Hide resolved
wdl-ast/src/validation.rs Outdated Show resolved Hide resolved
wdl-lint/tests/lints/double-quotes/source.wdl Outdated Show resolved Hide resolved
wdl-lint/tests/lints/nonmatching-output/source.errors Outdated Show resolved Hide resolved
wdl-lint/tests/lints/runtime-keys-wdl-1.0/source.errors Outdated Show resolved Hide resolved
wdl-lint/tests/lints/runtime-keys-wdl-1.1/source.errors Outdated Show resolved Hide resolved
wdl-lint/src/rules/misplaced_lint_directive.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/misplaced_lint_directive.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/unknown_rule.rs Outdated Show resolved Hide resolved
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.

Looks great!

@a-frantz a-frantz merged commit 2b45207 into main Sep 16, 2024
8 checks passed
@a-frantz a-frantz deleted the fix/lint-directives branch September 16, 2024 18:43
@a-frantz a-frantz mentioned this pull request Oct 31, 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.

Fix how exceptions work
2 participants