fix: redundant-none-rule #324 #333

prathmesh703 · 2025-03-04T15:14:29Z

START SECTION: adding a new linting rule

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

Rule Name: redundant-rule

fixes #324
added the lint rule that fires when = None is redundantly specified.

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.

Rule specific checks:

You have added the rule as an entry within RULES.md.
You have added the rule to the rules() function in wdl-lint/src/lib.rs.
You have added a test case in wdl-lint/tests/lints that covers every
possible diagnostic emitted for the rule within the file where the rule
is implemented.
[ ] If you have implemented a new Visitor callback, you have also
overridden that callback method for the special Validator
(wdl-ast/src/validation.rs) and LintVisitor
(wdl-lint/src/visitor.rs) visitors. These are required to ensure the new
visitor callback will execute.
You have run gauntlet --refresh to ensure that there are no
unintended changes to the baseline configuration file (Gauntlet.toml).
You have run gauntlet --refresh --arena to ensure that all of the
rules added/removed are now reflected in the baseline configuration file
(Arena.toml).

a-frantz · 2025-03-05T14:33:30Z

wdl-lint/src/rules/redundant_none.rs

+            wdl_ast::SyntaxKind::VersionStatementNode,
+            wdl_ast::SyntaxKind::WorkflowDefinitionNode,
+            wdl_ast::SyntaxKind::TaskDefinitionNode, 
+            wdl_ast::SyntaxKind::InputSectionNode,
+            wdl_ast::SyntaxKind::BoundDeclNode,


Suggested change

wdl_ast::SyntaxKind::VersionStatementNode,

wdl_ast::SyntaxKind::WorkflowDefinitionNode,

wdl_ast::SyntaxKind::TaskDefinitionNode,

wdl_ast::SyntaxKind::InputSectionNode,

wdl_ast::SyntaxKind::BoundDeclNode,

wdl_ast::SyntaxKind::VersionStatementNode,

wdl_ast::SyntaxKind::InputSectionNode,

wdl_ast::SyntaxKind::BoundDeclNode,

a-frantz · 2025-03-05T14:34:25Z

wdl-lint/src/rules/redundant_none.rs

+    }
+
+    fn tags(&self) -> TagSet {
+        TagSet::new(&[Tag::Style])


Suggested change

TagSet::new(&[Tag::Style])

TagSet::new(&[Tag::Clarity])

Personally, I'd tag it with both.

Yea that's true. It should have both the Clarity and Style tag to be consistent with other similar rules.

wdl-lint/src/rules/redundant_none.rs

wdl-lint/tests/lints/redudant-none/source.wdl

adthrasher · 2025-03-05T14:59:25Z

wdl-lint/src/rules/redundant_none.rs

+
+    fn explanation(&self) -> &'static str {
+        "Optional inputs with explicit None assignments can be simplified. For example, \
+         String? foo = None can be shortened to String? foo."


@a-frantz - I don't particularly like this snippet from the spec (the variable names, that is), but showing that the two are equivalent could be helpful as part of this text. I've made a simple edit suggestion below, but I'm curious if you think a longer explanation is preferable.

# the following are equivalent undefined optional declarations String? maybe_five_but_is_not String? also_maybe_five_but_is_not = None

Suggested change

String? foo = None can be shortened to String? foo."

`String? foo = None` is equivalent to and can be shortened to `String? foo`."

I'm ambivalent between your suggested edit and making it more verbose. I think the shorter text is "ok", but when the day comes that we create a dedicated website/wiki for all the lints, we might want to expand on it to include some "full" code snippets as examples. But given that we're currently only rendering this text in the terminal (from sprocket explain <rule>), brevity might be preferable?

Co-authored-by: Andrew Frantz <[email protected]>

…3/wdl into redundant-none-issue

wdl-lint/tests/lints/redudant-none/source.wdl

wdl-lint/src/rules/redundant_none.rs

a-frantz

please follow the instructions in the PR checklist to get this incorporated correctly. Your new rule isn't currently running, so we can't see what the diagnostics look like once rendered.

wdl-lint/src/rules/redundant_none.rs

Co-authored-by: Andrew Frantz <[email protected]>

prathmesh703 · 2025-03-09T03:41:44Z

@a-frantz please can you tell me where i'm getting wrong and what should i do ?

a-frantz · 2025-03-09T13:59:55Z

From the PR template checklist:

You have added the rule to the rules() function in wdl-lint/src/lib.rs.

This is needed so that your new rule can execute in the tests.

While you're at that, please go through the rest of the checklist and make any changes needed there.

a-frantz · 2025-03-18T15:27:46Z

please fix all the errors reported by our CI before further review

prathmesh703 · 2025-03-18T16:37:11Z

sure

…3/wdl into redundant-none-issue

claymcleod · 2025-03-23T20:24:04Z

wdl-lint/tests/lints/redudant-none/source.errors

+note[CallInputSpacing]: call input not properly spaced
+   ┌─ tests/lints/redudant-none/source.wdl:16:21
+   │  
+16 │       call test_task {


I'd fix this one and all of the non-relevant rules—we want tests to only show results for the specific rule that we are testing, not introduce unrelated warnings that need to be excepted.

claymcleod · 2025-03-23T20:24:38Z

wdl-lint/tests/lints/redudant-none/source.wdl

+    input {
+        String required_str
+        Int? optional_int  # should not flag, correct optional syntax
+        String? optional_str = None  # should flag, redundant None for optional


I don't see this one being flagged in the output.

@claymcleod sorry ! I will check it soon

a-frantz · 2025-03-24T16:29:10Z

https://github.com/stjude-rust-labs/wdl/blob/main/CONTRIBUTING.md#the-ci-has-turned-red-how-do-i-make-it-green-again

prathmesh703 · 2025-03-24T16:51:12Z

@a-frantz I will fix this soon

prathmesh703 · 2025-03-24T17:29:56Z

@a-frantz error: missing documentation for a module --> wdl-lint\src\rules.rs:36:1 | 36 | mod redundant_none; | ^^^^^^^^^^^^^^^^^^^
This error is coming on failing of the CI/lint
How to fix this type of error . Do I have to add documentation anywhere?

a-frantz · 2025-03-24T17:34:55Z

@a-frantz error: missing documentation for a module --> wdl-lint\src\rules.rs:36:1 | 36 | mod redundant_none; | ^^^^^^^^^^^^^^^^^^^ This error is coming on failing of the CI/lint How to fix this type of error . Do I have to add documentation anywhere?

Yes, you need to add documentation to everywhere that clippy reports is missing documentation. You can check locally by running cargo clippy --all-features and it will point out everywhere that needs to be documented.

a-frantz · 2025-03-24T17:35:23Z

https://github.com/stjude-rust-labs/wdl/blob/main/CONTRIBUTING.md#the-ci-has-turned-red-how-do-i-make-it-green-again

see this document for information about the failing checks

redundant-none-rule

c89e255

adthrasher requested review from adthrasher, claymcleod, a-frantz and peterhuene March 5, 2025 14:38

a-frantz reviewed Mar 5, 2025

View reviewed changes

a-frantz mentioned this pull request Mar 5, 2025

Fix redundant-none-rule implementation and correct tests #324 #336

Closed

11 tasks

adthrasher reviewed Mar 5, 2025

View reviewed changes

adthrasher assigned prathmesh703 Mar 5, 2025

prathmesh703 and others added 5 commits March 7, 2025 09:46

Merge branch 'main' into redundant-none-issue

9df5493

Update wdl-lint/src/rules/redundant_none.rs

6333912

Co-authored-by: Andrew Frantz <[email protected]>

Update wdl-lint/tests/lints/redudant-none/source.wdl

704ccf7

Co-authored-by: Andrew Frantz <[email protected]>

redundant-none fixes

40ebf1c

Merge branch 'redundant-none-issue' of https://github.com/prathmesh70…

4f5ef02

…3/wdl into redundant-none-issue

a-frantz reviewed Mar 7, 2025

View reviewed changes

wdl-lint/tests/lints/redudant-none/source.wdl Outdated Show resolved Hide resolved

a-frantz reviewed Mar 7, 2025

View reviewed changes

wdl-lint/src/rules/redundant_none.rs Outdated Show resolved Hide resolved

fixing reviews

8daa155

a-frantz reviewed Mar 8, 2025

View reviewed changes

wdl-lint/src/rules/redundant_none.rs Outdated Show resolved Hide resolved

wdl-lint/src/rules/redundant_none.rs Outdated Show resolved Hide resolved

prathmesh703 and others added 3 commits March 9, 2025 08:57

Update wdl-lint/src/rules/redundant_none.rs

812933e

Co-authored-by: Andrew Frantz <[email protected]>

Update wdl-lint/src/rules/redundant_none.rs

58c5f77

Co-authored-by: Andrew Frantz <[email protected]>

Merge branch 'main' into redundant-none-issue

5652d04

doxe-2611 and others added 3 commits March 10, 2025 22:47

added rules

e82de8a

adding rule

e0a2e25

Merge branch 'main' into redundant-none-issue

0dede65

Merge branch 'stjude-rust-labs:main' into redundant-none-issue

6e71c63

prathmesh703 marked this pull request as draft March 19, 2025 11:01

prathmesh703 changed the title ~~Fixes #324 redundant-none-rule~~ fix: redundant-none-rule #324 Mar 19, 2025

updated CHANGELOG.md

6fe9788

claymcleod added the S-awaiting-revisions State: awaiting revisions based on code review feedback label Mar 19, 2025

prathmesh703 and others added 4 commits March 20, 2025 22:28

Merge branch 'main' into redundant-none-issue

b960708

fix:format

4e751c7

Merge branch 'redundant-none-issue' of https://github.com/prathmesh70…

32f201e

…3/wdl into redundant-none-issue

Merge branch 'main' into redundant-none-issue

90afdce

prathmesh703 marked this pull request as ready for review March 22, 2025 05:25

claymcleod requested review from adthrasher and a-frantz March 23, 2025 20:22

claymcleod added S-needs-review State: awaiting initial or additional review and removed S-awaiting-revisions State: awaiting revisions based on code review feedback labels Mar 23, 2025

claymcleod reviewed Mar 23, 2025

View reviewed changes

peterhuene added the S-awaiting-pass-CI State: awaiting the CI to pass label Mar 25, 2025

claymcleod removed the S-needs-review State: awaiting initial or additional review label Mar 26, 2025

claymcleod force-pushed the main branch 3 times, most recently from 3dc8137 to f4c832c Compare March 26, 2025 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: redundant-none-rule #324 #333

fix: redundant-none-rule #324 #333

prathmesh703 commented Mar 4, 2025 •

edited

Loading

a-frantz Mar 5, 2025

a-frantz Mar 5, 2025

adthrasher Mar 5, 2025

a-frantz Mar 5, 2025

adthrasher Mar 5, 2025

a-frantz Mar 5, 2025

a-frantz left a comment

prathmesh703 commented Mar 9, 2025

a-frantz commented Mar 9, 2025

a-frantz commented Mar 18, 2025

prathmesh703 commented Mar 18, 2025

claymcleod Mar 23, 2025

claymcleod Mar 23, 2025

prathmesh703 Mar 24, 2025

a-frantz commented Mar 24, 2025

prathmesh703 commented Mar 24, 2025

prathmesh703 commented Mar 24, 2025

a-frantz commented Mar 24, 2025

a-frantz commented Mar 24, 2025

	String? foo = None can be shortened to String? foo."
	`String? foo = None` is equivalent to and can be shortened to `String? foo`."

fix: redundant-none-rule #324 #333

Are you sure you want to change the base?

fix: redundant-none-rule #324 #333

Conversation

prathmesh703 commented Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-frantz left a comment

Choose a reason for hiding this comment

prathmesh703 commented Mar 9, 2025

a-frantz commented Mar 9, 2025

a-frantz commented Mar 18, 2025

prathmesh703 commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-frantz commented Mar 24, 2025

prathmesh703 commented Mar 24, 2025

prathmesh703 commented Mar 24, 2025

a-frantz commented Mar 24, 2025

a-frantz commented Mar 24, 2025

prathmesh703 commented Mar 4, 2025 •

edited

Loading