Rules/too many required arguments #1176
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1176 +/- ##
==========================================
- Coverage 97.02% 96.97% -0.06%
==========================================
Files 37 39 +2
Lines 4670 5083 +413
==========================================
+ Hits 4531 4929 +398
- Misses 139 154 +15 ☔ View full report in Codecov by Sentry. |
…m/Lakitna/robotframework-robocop into rules/too-many-required-arguments
|
The failing test is caused by |
| for arg in arguments.values: | ||
| if "}=" in arg: |
There was a problem hiding this comment.
We may want to ignore errors as well, for example:
[Arguments] ${argument} = default
Not sure how it behaves but most likely it's parsing error, but may still come up here as 'optional' argument
There was a problem hiding this comment.
I did consider variations like this, but decided it was not worth the effort to explicitly support it. I even looked through the Userguide for this :) Now that I know where it is, it's an easy find:
The syntax for default values is space sensitive. Spaces before the = sign are not allowed, and possible spaces after it are considered part of the default value itself.
Source: https://robotframework.org/robotframework/latest/RobotFrameworkUserGuide.html#default-values-with-user-keywords
So } = is a parsing error:
[Arguments] ${argument} =defaultAnd = foo sets the default value of ${argument} to ${SPACE}foo:
[Arguments] ${argument}= fooTo me, syntax errors are out of scope while ${SPACE} is part of the value. Leading me to simply check for "}=".
Thought, writing this makes me think it can be good to add a rule to warn you about }= value 🤔 Could be called something like space-before-default-value.
There was a problem hiding this comment.
Yes, it will be parsing error - but we need to test it as it night be still parsed as argument and reported by the rule (just with a parsing error attached to node).
|
LGTM! Only minor comments. As for failing tests, I see it's test_all_builtin_rules_should_be_enabled_by_default - it can be ignroed (commented out even) as it will be deprecated in new robocop anyway. |
Co-authored-by: Bartłomiej Hirsz <[email protected]>
…m/Lakitna/robotframework-robocop into rules/too-many-required-arguments
|
My apologies - it looks like Github closed the PR after I started clean ups in the repository. I have changed master branch to main. Also, I have decided to keep merged Robocop inside this repository. I didn't want to lose all the stars, contributions and commit history after all! That means I will be copying the PRs from 'academy' repo into main branch here today. For your PR you have two options:
|
Closes #1145
It took a while due to other priorities, but here is the addition of 2 rules:
too-many-required-argumentstoo-many-optional-argumentsPlease note that I disabled
too-many-argumentsin favor of these new rules like discussed in the issue.