-
Notifications
You must be signed in to change notification settings - Fork 45
Fix unused global warnings from new flake8 7.2.0 #1609
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
Conversation
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.
Pull Request Overview
This PR fixes warnings raised by flake8 7.2.0 by removing unused nonlocal declarations and updates the copyright year.
- Updated copyright year from 2024 to 2025.
- Removed unused nonlocal variables (dir_patterns and file_patterns) from the inner function.
Comments suppressed due to low confidence (1)
user_tools/src/spark_rapids_pytools/common/sys_storage.py:242
- Confirm that the removal of 'dir_patterns' and 'file_patterns' from the nonlocal declaration does not affect any remaining references in the function. If these variables are no longer used, consider removing their declarations from the outer scope as well.
nonlocal files_count, dir_count
Signed-off-by: Partho Sarthi <[email protected]>
amahussein
left a comment
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.
Thanks @parthosa
QQ if this works on Py [3.9, 3.12]?
|
|
||
| def inner(dir_p: pathlib.Path, prefix: str = '', level=-1): | ||
| nonlocal files_count, dir_count, dir_patterns, file_patterns | ||
| nonlocal files_count, dir_count |
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 see dir_patterns and file_patterns are being used.
I guess it is because it is nort needed to be redefined as non_local.
Would this work for python 3.9+?
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.
That is correct. Since these are read-only variables (defined in the outer function), we do not need them to be non_local.
Yes, all tox tests passed for python versions 3.9-3.12.
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.
AFAIK, Tox is not a 100% reliable way to tell because if the unit-tests do not cover that path, we won't know that something is wrong in runtime.
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. That is true. In this case, I think it should be fine because these variables are read-only and these variables can be safely read in the inner function.
amahussein
left a comment
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 is interesting to see that co-pilot reviews are enabled on our repo.
Thanks @parthosa !
P.s: check the comment about Tox.
LGTME
Thanks @amahussein. I agree tox does not guarantee the coverage. But in this case, I think it should be fine because these variables are read-only and hence can be safely read in the inner function. |
Fixes #1608.
This PR fixes the code for a new warning added in the newly released flake8: PyCQA/pyflakes#825.