-
Notifications
You must be signed in to change notification settings - Fork 34
fix: redundant-none-rule #324 #333
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
Changes from 7 commits
c89e255
9df5493
6333912
704ccf7
40ebf1c
4f5ef02
8daa155
812933e
58c5f77
5652d04
e82de8a
e0a2e25
0dede65
6e71c63
6fe9788
b960708
4e751c7
32f201e
90afdce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
use crate::Rule; | ||
use crate::Tag; | ||
use crate::TagSet; | ||
use std::fmt::Debug; | ||
use wdl_ast::AstToken; | ||
use wdl_ast::Diagnostic; | ||
use wdl_ast::Diagnostics; | ||
use wdl_ast::Document; | ||
use wdl_ast::Span; | ||
use wdl_ast::SupportedVersion; | ||
use wdl_ast::SyntaxElement; | ||
use wdl_ast::VisitReason; | ||
use wdl_ast::Visitor; | ||
use wdl_ast::v1::InputSection; | ||
|
||
const ID: &str = "RedundantNoneAssignment"; | ||
|
||
fn redundant_none_assignment(span: Span, name: &str) -> Diagnostic { | ||
Diagnostic::note("redundant None assignment") | ||
.with_rule(ID) | ||
.with_highlight(span) | ||
.with_fix(format!("can be shortened to {}", name)) | ||
} | ||
|
||
#[derive(Default, Debug, Clone, Copy)] | ||
pub struct RedundantNoneAssignment; | ||
|
||
impl Rule for RedundantNoneAssignment { | ||
fn id(&self) -> &'static str { | ||
ID | ||
} | ||
|
||
fn description(&self) -> &'static str { | ||
"Flags redundant None assignments to optional inputs." | ||
} | ||
|
||
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." | ||
} | ||
|
||
fn tags(&self) -> TagSet { | ||
TagSet::new(&[Tag::Clarity]) | ||
} | ||
|
||
fn exceptable_nodes(&self) -> Option<&'static [wdl_ast::SyntaxKind]> { | ||
Some(&[ | ||
wdl_ast::SyntaxKind::VersionStatementNode, | ||
wdl_ast::SyntaxKind::InputSectionNode, | ||
wdl_ast::SyntaxKind::BoundDeclNode, | ||
]) | ||
} | ||
} | ||
|
||
impl Visitor for RedundantNoneAssignment { | ||
type State = Diagnostics; | ||
|
||
fn document( | ||
&mut self, | ||
_: &mut Self::State, | ||
reason: VisitReason, | ||
_: &Document, | ||
version: SupportedVersion, | ||
) { | ||
if reason == VisitReason::Exit { | ||
return; | ||
} | ||
*self = Default::default(); | ||
} | ||
|
||
fn input_section( | ||
&mut self, | ||
state: &mut Self::State, | ||
reason: VisitReason, | ||
section: &InputSection, | ||
) { | ||
if reason == VisitReason::Exit { | ||
return; | ||
} | ||
section.declarations().for_each(|decl| { | ||
if let token = decl.ty() { | ||
if token.is_optional() { | ||
if let Some(expr) = decl.expr() { | ||
if let Some(name_ref) = expr.as_literal().unwrap().as_none() { | ||
prathmesh703 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let text_range = decl.syntax().text_range(); | ||
let span = | ||
Span::from(text_range.start().into()..text_range.end().into()); | ||
state.exceptable_add( | ||
redundant_none_assignment(span, decl.name().as_str()), | ||
SyntaxElement::from(decl.syntax().clone()), | ||
&self.exceptable_nodes(), | ||
); | ||
prathmesh703 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} | ||
} | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
note[CallInputSpacing]: call input not properly spaced | ||
┌─ tests/lints/redudant-none/source.wdl:16:21 | ||
│ | ||
16 │ call test_task { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
│ ╭────────────────────^ | ||
17 │ │ input: | ||
│ ╰────────^ | ||
│ | ||
= fix: change this whitespace to a single space | ||
|
||
warning[MissingRuntime]: task `test_task` is missing a `runtime` section | ||
┌─ tests/lints/redudant-none/source.wdl:23:6 | ||
│ | ||
23 │ task test_task { | ||
│ ^^^^^^^^^ this task is missing a `runtime` section | ||
│ | ||
= fix: add a `runtime` section | ||
|
||
note[Whitespace]: line contains only whitespace | ||
┌─ tests/lints/redudant-none/source.wdl:31:1 | ||
│ | ||
31 │ | ||
│ ^^^^ | ||
│ | ||
= fix: remove the whitespace | ||
|
||
note[Whitespace]: line contains only whitespace | ||
┌─ tests/lints/redudant-none/source.wdl:35:1 | ||
│ | ||
35 │ | ||
│ ^^^^ | ||
│ | ||
= fix: remove the whitespace | ||
|
||
note[EndingNewline]: missing newline at the end of the file | ||
┌─ tests/lints/redudant-none/source.wdl:39:1 | ||
│ | ||
39 │ } | ||
│ ^ expected a newline to follow this | ||
│ | ||
= fix: add a newline at the end of the file | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
#@ except: DescriptionMissing, MissingRequirements | ||
#@ except: RuntimeSectionKeys, MissingMetas, MissingOutput | ||
|
||
version 1.0 | ||
|
||
workflow redundant_none_test { | ||
input { | ||
String required_str | ||
Int? optional_int # should not flag, correct optional syntax | ||
String? optional_str = None # should flag, redundant None for optional | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see this one being flagged in the output. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @claymcleod sorry ! I will check it soon |
||
Float? optional_float = 3.14 # should not flag, has non-None default | ||
Int required_int = 5 # should not flag | ||
} | ||
|
||
# Test in a task context as well | ||
call test_task { | ||
input: | ||
req_param = required_str, | ||
opt_param = optional_str, | ||
} | ||
} | ||
|
||
task test_task { | ||
input { | ||
String req_param | ||
Int? opt_int # should not flag | ||
File? opt_file = None # should not flag due to except directive | ||
String? opt_param = None # should flag, redundant None | ||
Boolean? opt_bool = true # should not flag, has non-None default | ||
} | ||
|
||
command <<< | ||
echo "Testing redundant None detection" | ||
>>> | ||
|
||
output { | ||
String result = "done" | ||
} | ||
} |
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.
@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.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.
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?