-
Notifications
You must be signed in to change notification settings - Fork 34
fix: shorten spans and remove some fixes #260
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
Conversation
I somehow managed to gloss over the one rule I specifically called out as an issue in #243 , but that's been corrected now 😅 I believe that now every problematically long span has been shortened. |
name.as_str(), | ||
runtime | ||
.syntax() | ||
.first_token() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this narrow it down to just the runtime
keyword? If so, you should update the expect()
statement to make it clear that there is always at least one token, which is the runtime
keyword.
self.current_meta_span = Some( | ||
section | ||
.syntax() | ||
.first_token() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here on the expect()
being clear (change unwrap()
to expect()
).
self.current_output_span = Some( | ||
section | ||
.syntax() | ||
.first_token() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
None => Some( | ||
section | ||
.syntax() | ||
.first_token() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
missing_trailing_comma( | ||
last_child | ||
.syntax() | ||
.last_token() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
missing_trailing_comma( | ||
last_child | ||
.syntax() | ||
.last_token() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
wdl-lint/src/rules/trailing_comma.rs
Outdated
@@ -261,7 +272,9 @@ impl Visitor for TrailingCommaRule { | |||
} else { | |||
// No comma found, report missing | |||
state.exceptable_add( | |||
missing_trailing_comma(last_child.text_range().to_span()), | |||
missing_trailing_comma( | |||
last_child.last_token().unwrap().text_range().to_span(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the above, I think you need a CHANGELOG.md
entry for the bug that you fixed on the multiple firing of that lint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just two nit comments!
Co-authored-by: Peter Huene <[email protected]>
│ | ||
= fix: use either "/n" or "/r/n" consistently in the file | ||
= fix: ensure that the same line endings (e.g., `/n` or `/r/n`) are used throughout the file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be backslashes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix diagnostics with backslashes in paths on Windows, the test baselines normalize (i.e. indiscriminately replaces all characters in the output) all backslashes to slashes, so it's expected the diagnostics are affected this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Good catch nvm, listen to peter lol. I didn't realize this was in the tests.
Describe the problem or feature in addition to a link to the issues.
closes #243
closes #193
Before submitting this PR, please make sure:
changes (when appropriate).
CHANGELOG.md
(see"keep a changelog" for more information).