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: add check for proper snake_case usage #13

Closed
wants to merge 12 commits into from

Conversation

simojoe
Copy link
Contributor

@simojoe simojoe commented Apr 12, 2024

This pull request adds a new rule to wdl-grammar.

  • Rule Name: snake_case
  • Rule Kind: Lint warning
  • Rule Code: v1::W006
  • Packages: wdl-grammar

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

This rule enforces snake case on workflows, tasks and variables names.

I had to modify other parts of the project to create this PR :

  • I changed the to_snake_case crate to the convert_case crate that will handle the same task (and more once the PascalCase rule gets implemented)
  • I added the task_name rule to mirror the workflow_name rule. This allows for a cleaner identification of the task names for this lint rule.

This rule has been created with an Array of rules to check which should be snake cased. This is to enable potential opt-in or opt-out of the various elements from the snake_case and PascalCase rules.

One caveat is that this rule does not check the imports (can we properly type them to differentiate the struct from the rest?)

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.

@a-frantz
Copy link
Member

Hi @simojoe! Thanks for the contribution! This PR is going to wait till we conclude the voting on #17.

(Our internal team has voted in concordance with how you've implemented things, but we want to get an idea of what the community is doing before cementing any decisions.)

@claymcleod claymcleod added the enhancement New feature or request label Apr 18, 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.

I think this looks really good! Looking at the results of the discussion above, I think this is probably going to be a fine baseline rule that we modify over time. As such, I gave some (rather picky) thoughts here.

wdl-core/src/concern/lint/group.rs Show resolved Hide resolved
wdl-grammar/CHANGELOG.md Outdated Show resolved Hide resolved
wdl-grammar/src/v1.rs Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/snake_case.rs Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/snake_case.rs Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/snake_case.rs Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/snake_case.rs Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/snake_case.rs Show resolved Hide resolved
wdl-grammar/src/v1/lint/snake_case.rs Show resolved Hide resolved
wdl-grammar/src/v1/wdl.pest Show resolved Hide resolved
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.

4 participants