Add full type safety to src/, drop Python 3.9 and Robot Framework 4.* support #1570
Conversation
…upport Add complete type annotations to src/ with strict mypy checking enabled. Minimum versions: Python 3.10+, Robot Framework 5.0+. Fix formatter parameter type conversion for config values.
…eptions and print issues
| self.up_to_column = int(up_to_column) - 1 | ||
| self.argument_indent = int(argument_indent) | ||
| self.min_width: int | None = int(min_width) if min_width is not None else None | ||
| self.fixed_width: int | None = int(fixed_width) if fixed_width is not None else None |
There was a problem hiding this comment.
I need to check it - I was under impression that we use typing to parse the input argument when creating instance of the class. But I even if its not the case, I agree that unclear typing made it ambigious.
There was a problem hiding this comment.
I've made quick and dirty test for it (I will need to add it to our tests later):
from robot.utils.importer import Importer
from robocop.formatter.formatters import resolve_args
from robocop.formatter.formatters.AlignKeywordsSection import AlignKeywordsSection
from robocop.formatter.skip import SkipConfig
def test_arguments_are_resolved_with_types():
args = {"compact_overflow_limit": "2"}
skip_config = SkipConfig()
importer = Importer()
spec = importer._get_arg_spec(AlignKeywordsSection)
positional, named, argument_names = resolve_args("AlignKeywordsSection", spec, args, skip_config, handles_skip={})
assert named["compact_overflow_limit"] == 2
So it does resolve the types. Hovewer if we're adding None it fails to resolve the type because of how the Robot Framework type resolver works (| None stops resolving the type). We have following options:
First, we can go with the current implementation (yours from the PR) since it's correct and works.
Second, in the future we may improve the type resolver - maybe possibly instead of using robot one, we can create our own as it's not that complex. Once it's done, we may be able to simplify this code to avoid double parsing the type.
OR
We can always assume that whatever we're passing is string, and leave converting the type to the formatter __init__ method. It's not that convenient and would be breaking change, that's why I am not considering it for now.
| if index < up_to: | ||
| if self.fixed_width: | ||
| return max(self.fixed_width - len(token.value), self.formatting_config.space_count) * " " | ||
| space_count = int(self.formatting_config.space_count) # type: ignore[union-attr,arg-type] |
There was a problem hiding this comment.
Instead of calling int on space count repeatedly, wouldnt it better to just ensure that it has proper type in formatting config? I saw few int() calls on it in the same file.
There was a problem hiding this comment.
Ok now I see where it's coming from - it's bit of poor design where class accept either value or None (so it should take default instead) and then it's converted in post processing. This is causing typing issues along the way. You can ignore my comment and leave it as it is and it can be addressed later (so improve design of this class -> replace those additional converts).
|
Thanks for the great work! I went through the changes and they are good in general. I saw not only pure typing changes but also code changes, but they usually are because we didnt have fallbacks to default values. It looks like ready to be merged (after rebasing and minor discussions) but I will wait with it until we release next robocop (7.1) since this change will go as 8.0. I need to add some documentation for 7.1, but it should be ready tommorrow. |
| if self.config.silent: | ||
| return | ||
| if hasattr(sys.stdout, "reconfigure"): | ||
| # reconfigure is available from Python 3.7+ |
There was a problem hiding this comment.
Since you noticed its available anyway, maybe we just need to remove this condition?
2 participants
Summary
This PR adds complete type annotations to the entire
src/robocop/codebase and enables strict Mypy checks with notype: ignorecomments. Additionally, it drops support for Python 3.9 and Robot Framework 4.*.Sorry, this is a large diff due to the nature of the change.
Changes
src/robocop/--strictflagBreaking Changes
Users must upgrade to: