-
Notifications
You must be signed in to change notification settings - Fork 22
tooling: Reduce 2 types of Pylance new warnings and more #873
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
tooling: Reduce 2 types of Pylance new warnings and more #873
Conversation
Fix pylance warnings for reportMissingTypeArgument and reportUnusedVariable and some other warnings. Also-by: Aymen Soussi [email protected]
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-check Status: Click to expand output
|
The created documentation from the pull request is available at: docu-html |
@@ -247,7 +248,7 @@ def draw_module( | |||
structure_text += f"}} /' {need['title']} '/ \n\n" | |||
|
|||
# Add logical interfaces only to implemented interfaces | |||
for iface, component in proc_impl_interfaces.items(): | |||
for iface, _ in proc_impl_interfaces.items(): |
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.
for iface, _ in proc_impl_interfaces.items(): | |
for iface in proc_impl_interfaces: |
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.
Fixed. Please resolve.
|
||
# from score_metamodel import ( | ||
# CheckLogger, | ||
# graph_check, | ||
# ) | ||
|
||
|
||
def get_standards_needs(needs: list[NeedsInfoType]) -> dict: | ||
def get_standards_needs(needs: list[dict[str, Any]]) -> dict[str, dict[str, Any]]: |
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.
Would it make sense to somehow extend NeedsInfoType with our keys? A generic dict is extremely unspecific.
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.
Might make sense to extend it or wrap it?
Just have to be careful to not end up with 200 custom NeedsInfoTypes Dataclasses in teh end.
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.
Maybe one in score_metamodel/__init__.py
?
https://chatgpt.com/share/67f4f048-6930-800a-8a49-b3a54cece1b4
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 guess it make more sence to do it in a seperate PR later if you agree guys as I tried to change some other files containing all_needs with NeedTypeInfo in state of list[dict[str, Any]] or other definitions but I end up getting more warnings and errors but for the file standards I already changed now with no side effects
@@ -35,7 +35,7 @@ def assert_no_warnings(self): | |||
) | |||
pytest.fail(f"Expected no warnings, but got:\n{warnings}") | |||
|
|||
def assert_warning(self, expected_substring: str, expect_location=True): | |||
def assert_warning(self, expected_substring: str, expect_location: bool = True): |
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.
Good example where annotating every parameter is useless :/
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, I agree.
@@ -67,7 +68,7 @@ | |||
needs_template_folder = "_templates" | |||
needs_global_options = {"collapse": True} | |||
html_static_path = ["_tooling/assets", "_assets"] | |||
needs_string_links = { | |||
needs_string_links: dict[str, dict[str, Any]] = { |
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.
was this required by any of the rules we have introduced? That was not intentional.
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.
It's role reportUnknownVariableType that throw this warning
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.
revert NeedsInfoType as discussed
Fix pylance warnings for reportMissingTypeArgument and reportUnusedVariable and some other warnings.
Also-by: Aymen Soussi [email protected]
Related to: #836