-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Configured mypy strict for nav2_smac_planner #5022
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 all commits
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 |
|---|---|---|
|
|
@@ -12,3 +12,11 @@ write-changes = false | |
| profile = "google" | ||
| force_single_line = false | ||
| line_length = 99 | ||
|
|
||
| [tool.mypy] | ||
| explicit_package_bases = true | ||
| strict = true | ||
|
|
||
| [[tool.mypy.overrides]] | ||
| module = ["matplotlib.*", "rtree.*"] | ||
| ignore_missing_imports = true | ||
|
Contributor
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. Why is this needed? If that is set globally we are weakening the static type checking very much (see mypy doc. If there are imports that do not support typing, they should be ignored by adding the following to pyproject.toml:
Contributor
Author
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. Thank you for the tip! |
||
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 am a bit surprised that this is not part of the ament_mypy configuration. Maybe we should open a PR regarding this configuration setting in the ament_lint repo.
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 agree if you think its good to include!
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 don't have a strong feeling one way or another on this one, should we keep it on?
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.
The default ament_mypy_configuration has
ignore_missing_imports = true. I have additionally addedexplicit_package_bases = trueto treat each package independently.I think this can be added as a PR for the ament_lint repository.
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 think we can keep this file, to include
strict = truefor strict type checking.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 think these 2 additions sound good to me. Just waiting for @Nils-ChristianIseke's OK
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.
Yes let use strict = True. However that will result in many mypy errors, so you just have to configure mypy to only check the packages that have type hints (currently only nav2_smac_planner). An you will most certanly have to tweak the type hints of nav2_smac_planner.