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

revise: preamble and version stmt lint rules #187

Merged
merged 34 commits into from
Oct 1, 2024
Merged

Conversation

a-frantz
Copy link
Member

@a-frantz a-frantz commented Sep 25, 2024

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

This revises the preamble+version formatting rules to use some more whitespace. PreambleComments and PreambleWhitespace have been refactored into 3 rules: PreambleFormatting, VersionFormatting, and PreambleCommentAfterVersion.

The tests have been cleaned up. This means using lint directives judiciously and generally following more of our own lints within the tests.

Some rules that were emitting warnings have been downgraded to notes.

Many TODOs have been logged. We generally prefer logging issues to TODO comments, but I didn't want to flood the issues with a bunch of tiny complaints. Maybe that's preferred?

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 Sep 26, 2024
@a-frantz a-frantz changed the title Revise/preamble revise: preamble and version stmt lint rules Sep 27, 2024
@a-frantz a-frantz marked this pull request as ready for review September 30, 2024 17:47
@peterhuene
Copy link
Collaborator

Many TODOs have been logged. We generally prefer logging issues to TODO comments, but I didn't want to flood the issues with a bunch of tiny complaints. Maybe that's preferred?

I think it's find to create either a single issue to track the various TODOs as an item in a check list or to track each independently if it makes sense to do.

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 good! Just a few feedback comments.

I also tried to comment on the TODOs where encountered to act as a reminder to create tracking issues / a singular tracking issue; I might have missed some.

wdl-lint/src/rules/nonmatching_output.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/preamble_comment_after_version.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/preamble_formatting.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/preamble_formatting.rs Outdated Show resolved Hide resolved
wdl-lint/src/rules/version_formatting.rs Outdated Show resolved Hide resolved
wdl-lint/tests/lints/one-line-after-version/source.wdl Outdated Show resolved Hide resolved
wdl-lint/tests/lints/preamble-ws/source.wdl Outdated Show resolved Hide resolved
wdl-lint/tests/lints/runtime-keys-wdl-1.2/source.wdl Outdated Show resolved Hide resolved
wdl-lint/tests/lints/trailing-comma/source.wdl Outdated Show resolved Hide resolved
@a-frantz
Copy link
Member Author

a-frantz commented Oct 1, 2024

Many TODOs have been logged. We generally prefer logging issues to TODO comments, but I didn't want to flood the issues with a bunch of tiny complaints. Maybe that's preferred?

I think it's find to create either a single issue to track the various TODOs as an item in a check list or to track each independently if it makes sense to do.

opened #203 with 6 items to track

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 good!

@a-frantz a-frantz merged commit 6c8457a into main Oct 1, 2024
8 checks passed
@a-frantz a-frantz deleted the revise/preamble branch October 1, 2024 17:40
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.

2 participants