feat: Enable processing of empty values in multiple sections (variables, keywords, test cases)#1719
Conversation
…es, keywords, test cases)
|
I see there are some errors for older versions, I'll look into that |
|
No reason it wasn't enabled for other sections. Since it has sibling rule |
| configure = [ | ||
| "ReplaceEmptyValues.sections=all", | ||
| # or | ||
| "ReplaceEmptyValues.sections=['variables','keywords']", |
There was a problem hiding this comment.
Our common approach for such parameter was to use csv and then process it later (split on comma). In this case it's not clear what will be the final type - most likely the string anyway, so using the 'string in form of list' is not ideal. The parsing will be also the same for both toml and cli config.
ReplaceEmptyValues.sections=variables,keywords
There was a problem hiding this comment.
Updated to csv style! Good to stay consistent. I was under the assumption in my head that convention was a list, but clearly it's not. Although, I still have to update documentation
|
|
||
| @skip_if_disabled | ||
| def visit_Variable(self, node: Variable) -> Variable: # noqa: N802 | ||
| if self.current_section != "variables": |
There was a problem hiding this comment.
Do we need to track whether we're inside variables? VAR would be visit_Var, so the visit_Variable should only apply to variables section. But I'm not 100 % sure
There was a problem hiding this comment.
This was for some debugging I did while building it. I removed it
| def visit_Var(self, node: Var) -> Var: # noqa: N802 | ||
| """Handle inline VAR statements to replace empty values with proper EMPTY variables.""" | ||
| if self.current_section not in ("testcases", "keywords"): | ||
| return node |
There was a problem hiding this comment.
VAR may only exist in testcases / keywords section so this statement will be always false.
| """Handle inline VAR statements to replace empty values with proper EMPTY variables.""" | ||
| if self.current_section not in ("testcases", "keywords"): | ||
| return node | ||
| if Var is None or node.errors: |
There was a problem hiding this comment.
Var is None probably will never be true (because visit_Var is only handled by version which supports Var) but it may be required for mypy checker.
|
|
||
| args = node.get_tokens(Token.ARGUMENT) | ||
| if args: | ||
| return node |
There was a problem hiding this comment.
What about:
VAR ${name}
...
There is arg, but it's empty.
There was a problem hiding this comment.
Good one. I didn't think of multilines. It is now implemented!
- add csv style config for the formatter - add support for multiline empty Variables - update tests to match multiline empty variables
|
I still have a few comments to check, and to see why some of the tests are failing on older python version. Will let you know when I have time for that! |
2 participants
Hey,
No idea if there was a specific reason to only enable the ReplaceEmptyValues rule in the variables section. So I created this PR with the functionality to also have it in Test Cases, and Keywords.
For now, I created it with only variables as the default section, so core behaviour doesn't change. You can now add a specific setting that enables you to choose which sections as a comma separated list (e.g. "variables,keywords"), and including "all".
Please let me know any changes you would like to see!
BR,
Mark