-
-
Notifications
You must be signed in to change notification settings - Fork 375
python: replace mutable default arguments (W0102) and enable unused wildcard import check (W0614) #6669
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
echoix
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.
For all the other changes, like the ones for topolist, I think it would make sense to edit the docstrings to explain that the default value of xxxx is used when None is given (None isn't really an accepted value).
Especially since the function signature doesn't give a clue anymore on this.
| global _MSGR_INSTANCE | ||
| if _MSGR_INSTANCE is None: | ||
| _MSGR_INSTANCE = Messenger(*args, **kwargs) | ||
| return _MSGR_INSTANCE |
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 really like the approach used here. I think it shows well how the previous code was abusing the mutability of the default value, and was affected by it (it was replacing the element at index 0 probably because of that).
However, global variables are usually a source of problems, harder to reason correctly about them. Now, is it worse than the dangerous default value, probably not understood?
I don't understand extremely well the pygrass message module, so I'd like some help here
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 that the global variable approach can introduce complexity, and the previous code’s mutable default was indeed problematic. My intent was to quickly fix the W0102 issue with a straightforward pattern, but I see the benefit of encapsulating this in a class method or decorator for better clarity and maintainability.
I can update the code to use a class method singleton or a decorator pattern if that’s preferred.
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 any special insights to it, but I think this is at least more readable than the previous code. The "instance" parameter doesn't seem to be used anywhere, so I think it's fine to get rid of it.
|
Thank you for the detailed feedback! Regarding the docstrings, I will add clarifications explicitly stating that when the parameter is None, a default list (e.g., ['EQUAL']) is created internally. This should make the behavior clear despite the function signature no longer showing the mutable default. |
echoix
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.
Do you have a better idea for how to only solve what we have here, without addressing the 3041 issues in the gui folder?
Appart from not un commenting it...
Does that rule exist in ruff that could be more easily configured there to help with checking most of the cases?
|
Oh, ruff PLW0102 doesn't exist, because B006 is implemented and does the same job. So try making CI pass by enabling B006 (excluding the gui folder) |
It seems to be about 30 files, I don't know what the 3041 mean. |
Locally, he only ran for the grass package, but we actually have three parts, the grass python api, the tools written in python in the script folder, and the gui. He solved only in the grass package, but there are lots of remaining issues in the gui folder. Removing the excluded rule enabled it for everywhere. |
Pylint confirms no W0102 warnings remain, and W0614 checks are now active to catch future issues.