-
Notifications
You must be signed in to change notification settings - Fork 37
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
Rule: inconsistent-args
#448
Conversation
9d4f9c4
to
88d8e29
Compare
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.
👍
# regal ignore:prefer-some-in-iteration | ||
name := ast.ref_to_string(ast.functions[i].head.ref) | ||
args_list := [args | | ||
some j |
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.
Probably won't make a dent on performance, but we could add a i != j
here, right?
# regal ignore:prefer-some-in-iteration | ||
args := ast.functions[j].head.args | ||
] | ||
count(args_list) > 1 |
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.
Won't the arg list size always be > 1, as each rule is counted at least twice, once when iterating i
and once for j
?
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.
But the args_list only iterates on j
.
|
||
# regal ignore:prefer-some-in-iteration | ||
name := ast.ref_to_string(ast.functions[i].head.ref) | ||
args_list := [args | |
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.
We'd possibly get a perf boost by making args_list
a set.
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.
If we didn't need to account for primitives, we could then simply just have made a count(args_list) > 1
check 😄 .
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.
We still need to iterate over it, so I don't think a set would perform better? 🤔
|
||
# Return the _second_ function found by name, as that | ||
# is reasonably the location the inconsistency is found | ||
find_function_by_name(name) := [fn | |
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.
Unless we can a pinpoint the first function with an actual argument name inconsistency, isn't it more clear to just point to the first function declaration.
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.
Possibly, but the second function is at least arguably more likely to be inconsistent than the first :P
Warn on inconsistent argument names in repeated function declarations. Fixes #444 Signed-off-by: Anders Eknert <[email protected]>
88d8e29
to
05a10f0
Compare
Warn on inconsistent argument names in repeated function declarations. Fixes StyraInc#444 Signed-off-by: Anders Eknert <[email protected]>
Warn on inconsistent argument names in repeated function declarations.
Fixes #444