Improve MixedIndentation rule to always emit notes for … #401

Gyan-max · 2025-04-08T03:48:23Z

…document-level indentation issues and warnings for command sections

START SECTION:

This PR improves the MixedIndentation rule to consistently detect and report mixed indentation issues throughout WDL documents.

The improvement includes:

Emitting notes for document-level mixed indentation consistently throughout the entire document.
Continuing to check for more instances of mixed indentation after finding the first occurance
Updated the RULES.md to reflect the functionality

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 PR title follows 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 --bless to ensure that there are no
unintended changes to the baseline configuration file (Gauntlet.toml).
You have run gauntlet --bless --arena to ensure that all of the
rules added/removed are now reflected in the baseline configuration file
(Arena.toml).

Close #201

…document-level indentation issues and warnings for command sections

…rule changes

…nges

adthrasher · 2025-04-08T15:24:18Z

@Gyan-max - Please complete the PR checklist.

wdl-lint/RULES.md

wdl-lint/src/rules/command_mixed_indentation.rs

adthrasher · 2025-04-08T15:35:31Z

Gauntlet.toml

@@ -578,7 +578,7 @@ permalink = "https://github.com/aws-samples/amazon-omics-tutorials/blob/c624acf2

 [[diagnostics]]
 document = "biowdl/tasks:/bcftools.wdl"
-message = 'bcftools.wdl:114:75: error: unknown escape sequence `\_`'
+message = "bcftools.wdl:114:75: error: unknown escape sequence `/_`"


This isn't for this PR, but this escape is in a parameter_meta entry. I suspect the associated rule is too broad.

yes,
It's incorrectly flagging valid characters like \_ and /_ as invalid escape sequences in parameter metadata descriptions.
I don't know why but these characters are valid in parameter metadata description and i think they should not be flagged as errors.

wdl-lint/src/rules/command_mixed_indentation.rs

…t cases

adthrasher · 2025-04-08T17:53:00Z

@Gyan-max - Please complete the PR checklist.

I guess I need to clarify. The following three items are checked in your checklist above, but are not done. Please fix them.

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.

Gyan-max · 2025-04-08T18:00:58Z

@Gyan-max - Please complete the PR checklist.

I guess I need to clarify. The following three items are checked in your checklist above, but are not done. Please fix them.

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.

Sorry for the confusion in the checklist.
Can you guide me on how can I add assignee and code reviewers
like do I have to mention them or what?

adthrasher · 2025-04-08T18:24:23Z

Gyan-max · 2025-04-09T00:07:13Z

I think I don't have permission to add assignee and code reviewer.

a-frantz · 2025-04-09T00:08:57Z

I think I don't have permission to add assignee and code reviewer.

that's good to know, thanks. You still need to fix the failing tests though. We can handle the assigning portions.

Gyan-max · 2025-04-09T01:50:23Z

@a-frantz I don't know why there are so many Cl failed tests, initially there was only 3, can you please review my commits and guide me how to fix them

adthrasher · 2025-04-09T13:20:12Z

wdl-lint/src/rules/command_mixed_indentation.rs

-use crate::Tag;
-use crate::TagSet;
+use crate::tags::Tag;
+use crate::tags::TagSet;
 use crate::util::lines_with_offset;

 /// The identifier for the command section mixed indentation rule.


Suggested change

/// The identifier for the command section mixed indentation rule.

/// The identifier for the mixed indentation rule.

adthrasher · 2025-04-09T13:24:27Z

wdl-lint/tests/lints/command-mixed-line-cont/source.errors

-   │     ------- this command section uses both tabs and spaces in leading whitespace
-13 │         this line has a continuation /
-14 │            and should be a warning
-   │  ^^^ indented with spaces until this tab


This is a regression. This rule should flag the position where the spacing changed types.

adthrasher · 2025-04-09T13:27:34Z

wdl-lint/tests/lints/document-and-command-mixed-indentation/source.errors

Where is the associated test file?

adthrasher · 2025-04-09T13:27:47Z

wdl-lint/tests/lints/document-mixed-indentation/source.errors

adthrasher · 2025-04-09T13:28:44Z

wdl-lint/tests/lints/mixed-indentation/source.errors

These lints need to get cleaned up if they're not relevant to what is being tested.

adthrasher · 2025-04-09T13:29:25Z

wdl-lint/tests/lints/only-whitespace/source.errors

@@ -30,6 +30,14 @@ note[Whitespace]: line contains only whitespace
   │
   = fix: remove the whitespace

+note[CommandSectionMixedIndentation]: mixed indentation throughout document


This isn't a command section, so the rule name is incorrect.

adthrasher · 2025-04-09T13:30:31Z

wdl-lint/tests/lints/within-import-whitespace/source.errors

Nothing should have changed in this file.

…of indentation changes

…iles

…yan-max/wdl into fix-document-mixed-indentation

Gyan-max · 2025-04-09T16:14:29Z

@adthrasher I tried to address all your feedback,
Please review the commits

…ibility

wdl-lint/src/rules/command_mixed_indentation.rs

adthrasher · 2025-04-09T18:34:06Z

wdl-lint/src/lib.rs

Where did these changes come from?

adthrasher · 2025-04-09T18:34:31Z

wdl-lint/src/rules.rs

+/// Rules for detecting mixed indentation (spaces and tabs) in WDL command
+/// sections and documents.


Suggested change

/// Rules for detecting mixed indentation (spaces and tabs) in WDL command

/// sections and documents.

There are no other comments in this list.

…rror files

adthrasher

Please respond in writing to the review comments before pushing additional changes

adthrasher · 2025-04-10T17:29:01Z

wdl-lint/src/rules.rs

@@ -2,6 +2,7 @@

 mod blank_lines_between_elements;
 mod call_input_spacing;
+#[doc = "Module for detecting mixed indentation in command sections and documents."]


This is the second time you've added a comment to this file. Instead of deleting it, can you reply and explain why you made this change?

adthrasher · 2025-04-10T17:29:35Z

wdl-lint/src/lib.rs

Why did this file change? It looks like you just reordered these entries, what was the motivation behind that decision?

adthrasher · 2025-04-10T17:29:58Z

ci/src/main.rs

@@ -334,7 +334,7 @@ fn bump(version: &str, patch_bump: bool) -> String {

 /// Publishes a crate.
 async fn publish(name: &str, version: &str, manifest_path: &Path, dry_run: bool) -> bool {
-    if !SORTED_CRATES_TO_PUBLISH.iter().any(|s| *s == name) {
+    if !SORTED_CRATES_TO_PUBLISH.contains(&name) {


Why did this change?

adthrasher · 2025-04-10T17:31:05Z

wdl-lint/src/rules/command_mixed_indentation.rs

+/// Type alias for backward compatibility
+pub type CommandSectionMixedIndentationRule = MixedIndentationRule;


Why are we concerned about backwards compatibility here?

adthrasher · 2025-04-10T17:31:59Z

wdl-lint/src/rules/command_mixed_indentation.rs

+#[derive(Default, Debug, Clone)]
+pub struct MixedIndentationRule {
+    /// The visitor that does the actual work.
+    visitor: MixedIndentationVisitor,


This is a departure from all of the existing lint rules. Why does this rule need a Visitor member instead of simply implementing the trait for the rule?

adthrasher · 2025-04-10T17:33:12Z

wdl-lint/src/rules/command_mixed_indentation.rs

    }

    fn related_rules(&self) -> &[&'static str] {
-        &[]
+        &["Whitespace"]


How is the Whitespace rule related to this rule?

adthrasher · 2025-04-10T17:33:39Z

wdl-lint/tests/lints/within-import-whitespace/source.errors

Why did this file change?

Gyan-max · 2025-04-10T18:29:40Z

@adthrasher Sorry for soo much commits, I was just trying to fix the Cl failed tests and in order to fix it I somewhere messed up with code logic and implementation.
I need your guidance for Cl test failures.
and i again request you to please review my code and suggest me fixes that I should make in order to implement this rule.

adthrasher · 2025-04-10T18:41:54Z

@adthrasher Sorry for soo much commits, I was just trying to fix the Cl failed tests and in order to fix it I somewhere messed up with code logic and implementation. I need your guidance for Cl test failures. and i again request you to please review my code and suggest me fixes that I should make in order to implement this rule.

Before we go any further, please respond to the questions I left in my last review. This PR has grown to a much larger set of changes than I would expect and I want to understand how it got to be that way. So if you could explain the changes you've made so far, that'd be great.

Gyan-max · 2025-04-10T18:47:48Z

@adthrasher Sorry for soo much commits, I was just trying to fix the Cl failed tests and in order to fix it I somewhere messed up with code logic and implementation. I need your guidance for Cl test failures. and i again request you to please review my code and suggest me fixes that I should make in order to implement this rule.

Before we go any further, please respond to the questions I left in my last review. This PR has grown to a much larger set of changes than I would expect and I want to understand how it got to be that way. So if you could explain the changes you've made so far, that'd be great.

Sure

Gyan-max · 2025-04-10T19:11:41Z

I think I messed up the whole PR, just to resolve the Cl test failure. If you allow can I make a fresh start from scratch?

adthrasher · 2025-04-10T20:02:48Z

Per @Gyan-max, closing this PR.

Gyan-max added 5 commits April 8, 2025 08:46

Improve CommandSectionMixedIndentation rule to always emit notes for …

0893045

…document-level indentation issues and warnings for command sections

docs: update CHANGELOG.md and RULES.md to document mixed indentation …

8b196f8

…rule changes

chore: update Gauntlet.toml baseline

c72b9c0

style: apply cargo fmt to command_mixed_indentation.rs

b5b6dab

test: update test expectations for CommandSectionMixedIndentation cha…

10b5446

…nges

adthrasher reviewed Apr 8, 2025

View reviewed changes

wdl-lint/RULES.md Outdated Show resolved Hide resolved

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

adthrasher reviewed Apr 8, 2025

View reviewed changes

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

adthrasher reviewed Apr 8, 2025

View reviewed changes

wdl-lint/src/rules/command_mixed_indentation.rs Show resolved Hide resolved

Gyan-max and others added 2 commits April 8, 2025 23:15

Merge branch 'main' into fix-document-mixed-indentation

8db9498

Rename CommandSectionMixedIndentation to MixedIndentation and add tes…

067342f

…t cases

github-actions bot added the S-awaiting-CI State: awaiting the CI to pass label Apr 8, 2025

a-frantz assigned Gyan-max Apr 9, 2025

a-frantz mentioned this pull request Apr 9, 2025

docs: revise part of checklist that external contributors can't do #405

Merged

9 tasks

Gyan-max changed the title ~~Improve CommandSectionMixedIndentation rule to always emit notes for …~~ Improve MixedIndentation rule to always emit notes for … Apr 9, 2025

Gyan-max added 2 commits April 9, 2025 07:01

Fix MixedIndentationRule implementation to match Rule trait interface

540b7af

Update test error files to match expected format

b934521

adthrasher requested changes Apr 9, 2025

View reviewed changes

adthrasher requested review from peterhuene, claymcleod and a-frantz April 9, 2025 13:37

Gyan-max and others added 2 commits April 9, 2025 20:00

Merge branch 'main' into fix-document-mixed-indentation

06f7194

Update the mixed indentation tests to be specific about the position …

3bd2dd8

…of indentation changes

Gyan-max added 2 commits April 9, 2025 21:41

Fix rule name in only-whitespace and cleanup mixed-indentation test f…

0bbee71

…iles

Merge branch 'fix-document-mixed-indentation' of https://github.com/G…

331e540

…yan-max/wdl into fix-document-mixed-indentation

Gyan-max added 3 commits April 9, 2025 21:58

Fix mixed indentation rule implementation and add module documentation

66956a3

Fix CommandSectionMixedIndentationRule reference in lib.rs

b1aaef4

Fix CommandSectionMixedIndentationRule type alias for backward compat…

3655128

…ibility

adthrasher mentioned this pull request Apr 9, 2025

Unknown escape sequence rule is too broad #406

Open

Fix comment line formatting in wdl-lint/src/lib.rs

a3d9bed

adthrasher reviewed Apr 9, 2025

View reviewed changes

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

Standardize rule ID naming in command_mixed_indentation.rs

a974616

adthrasher reviewed Apr 9, 2025

View reviewed changes

Gyan-max added 2 commits April 10, 2025 08:14

Remove doc comment from command_mixed_indentation module and update e…

cb9f501

…rror files

Fix module documentation and test files

b74c550

adthrasher reviewed Apr 10, 2025

View reviewed changes

adthrasher closed this Apr 10, 2025

Gyan-max deleted the fix-document-mixed-indentation branch April 11, 2025 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve MixedIndentation rule to always emit notes for … #401

Improve MixedIndentation rule to always emit notes for … #401

Gyan-max commented Apr 8, 2025 •

edited

Loading

adthrasher commented Apr 8, 2025

adthrasher Apr 8, 2025

Gyan-max Apr 8, 2025

adthrasher commented Apr 8, 2025

Gyan-max commented Apr 8, 2025

adthrasher commented Apr 8, 2025

Gyan-max commented Apr 9, 2025

a-frantz commented Apr 9, 2025

Gyan-max commented Apr 9, 2025

adthrasher Apr 9, 2025

adthrasher Apr 9, 2025

adthrasher Apr 9, 2025

adthrasher Apr 9, 2025

adthrasher Apr 9, 2025

adthrasher Apr 9, 2025

adthrasher Apr 9, 2025

Gyan-max commented Apr 9, 2025

adthrasher Apr 9, 2025

adthrasher Apr 9, 2025

adthrasher left a comment

adthrasher Apr 10, 2025

adthrasher Apr 10, 2025

adthrasher Apr 10, 2025

adthrasher Apr 10, 2025

adthrasher Apr 10, 2025

adthrasher Apr 10, 2025

adthrasher Apr 10, 2025

Gyan-max commented Apr 10, 2025

adthrasher commented Apr 10, 2025

Gyan-max commented Apr 10, 2025

Gyan-max commented Apr 10, 2025 •

edited

Loading

adthrasher commented Apr 10, 2025

	/// The identifier for the command section mixed indentation rule.
	/// The identifier for the mixed indentation rule.

		/// Rules for detecting mixed indentation (spaces and tabs) in WDL command
		/// sections and documents.

		/// Type alias for backward compatibility
		pub type CommandSectionMixedIndentationRule = MixedIndentationRule;

Improve MixedIndentation rule to always emit notes for … #401

Improve MixedIndentation rule to always emit notes for … #401

Conversation

Gyan-max commented Apr 8, 2025 • edited Loading

adthrasher commented Apr 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adthrasher commented Apr 8, 2025

Gyan-max commented Apr 8, 2025

adthrasher commented Apr 8, 2025

Gyan-max commented Apr 9, 2025

a-frantz commented Apr 9, 2025

Gyan-max commented Apr 9, 2025

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

Choose a reason for hiding this comment

Gyan-max commented Apr 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adthrasher left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gyan-max commented Apr 10, 2025

adthrasher commented Apr 10, 2025

Gyan-max commented Apr 10, 2025

Gyan-max commented Apr 10, 2025 • edited Loading

adthrasher commented Apr 10, 2025

Gyan-max commented Apr 8, 2025 •

edited

Loading

Gyan-max commented Apr 10, 2025 •

edited

Loading