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: adds the one_empty_line lint warning #33

Merged
merged 11 commits into from
May 14, 2024

Conversation

simojoe
Copy link
Contributor

@simojoe simojoe commented May 10, 2024

This pull request adds a new rule to wdl.

  • Rule Name: one_empty_line
  • Rule Kind: Lint warning
  • Rule Code: v1::W011
  • Packages: wdl-grammar

Parses the file line by line, trims whitespace and emits one warning per block of 2 or more continuous empty lines.

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).

a-frantz and others added 2 commits May 13, 2024 09:14
Co-authored-by: Andrew Thrasher <adthrasher@gmail.com>
Co-authored-by: Andrew Frantz <andrew.frantz@stjude.org>
wdl-grammar/CHANGELOG.md Outdated Show resolved Hide resolved
wdl-grammar/src/v1/lint/one_empty_line.rs Outdated Show resolved Hide resolved
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.

Looks good to me aside from what @adthrasher pointed out. I have the same question, but once that's resolved I give the all clear

@simojoe
Copy link
Contributor Author

simojoe commented May 13, 2024

Good point! Modify to actually tag the last infringing line. Since this warning requires as least two lines to report, this should always unwrap successfully.

@simojoe simojoe requested a review from adthrasher May 14, 2024 18:34
@adthrasher
Copy link
Member

@a-frantz - This now reports the last problematic blank line. Do you think that is sufficient or should it flag every extra blank line?

I was thinking something like this, so that every empty line would be flagged.

        for (line_no, start_byte_no, _end_byte_no, line) in tree.as_str().lines_with_offsets() {
            if line.trim().is_empty() {
                if n_empty_lines >= 1 {
                    warnings.push_back(self.more_than_one_empty_line(Location::Position(
                        Position::new(
                            NonZeroUsize::try_from(line_no.get()).unwrap(),
                            NonZeroUsize::try_from(1).unwrap(),
                            start_byte_no,
                         ),
                     )));
                }
                n_empty_lines += 1;
            }  else {
                n_empty_lines = 0;
            }
        }

@adthrasher adthrasher requested a review from a-frantz May 14, 2024 20:10
@adthrasher adthrasher requested a review from a-frantz May 14, 2024 20:31
@simojoe
Copy link
Contributor Author

simojoe commented May 14, 2024

I would be happy modifying my PR as to fit the expectations.

The one lint warning per block of erroneous empty lines design choice was defined in the original post as it was not previously defined in the description.

The EOF check was not part of the PR?

@adthrasher
Copy link
Member

I would be happy modifying my PR as to fit the expectations.

The one lint warning per block of erroneous empty lines design choice was defined in the original post as it was not previously defined in the description.

The EOF check was not part of the PR?

I made a change to report a Span so that the entire set of extraneous whitespace is reported.

As for the EOF, @a-frantz and I discussed it afterward and he pointed out your other PR for newline_eof. I had simply forgotten that was a separate rule.

Thanks for your contributions, @simojoe!

@adthrasher adthrasher merged commit 82eeb0f into stjude-rust-labs:main May 14, 2024
5 of 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