Skip to content
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

python: Replace usages of Python2-only import __builtin__ with a Python 3 compatible equivalent #3329

Merged
merged 2 commits into from
Jan 28, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Jan 4, 2024

Fixes #3328

I took the approach of setting in the dict, as tools like Pyright complain (rightfully), that _ isn't a know builtin. It's true, its something a bit more dynamic. That's about what we use for the normal translations in

import builtins as _builtins

# Initialize the translation attribute of the translate function to indicate
# that the translations are not initialized.
_translate.translation = None
_builtins.__dict__["_"] = _translate

@github-actions github-actions bot added GUI wxGUI related Python Related code is in Python libraries labels Jan 4, 2024
gui/wxpython/core/toolboxes.py Outdated Show resolved Hide resolved
@neteler neteler added this to the 8.4.0 milestone Jan 14, 2024
@echoix echoix force-pushed the fix-python2-builtin branch from 9d8c953 to 99234b4 Compare January 14, 2024 13:45
@github-actions github-actions bot added the docs label Jan 14, 2024
@echoix echoix dismissed wenzeslaus’s stale review January 14, 2024 13:47

Addressed sugestions to not keep older aliased name

@echoix
Copy link
Member Author

echoix commented Jan 14, 2024

Ready to review

@ninsbl
Copy link
Member

ninsbl commented Jan 23, 2024

Looks fine, but I wonder: why didn't it fail? Is this dead code?
Or is it wrapped in other Python 2/3 compatibility code that could be removed?

@echoix
Copy link
Member Author

echoix commented Jan 23, 2024

Looks fine, but I wonder: why didn't it fail? Is this dead code? Or is it wrapped in other Python 2/3 compatibility code that could be removed?

It's simple: It isn't touched in any of the pytest or gunittest tests that run in the ubuntu and pytest workflows. If you want to explore a bit on the covered lines in what will be an upcoming PR, once I'm satisfied with the config, you can explore here: https://app.codecov.io/gh/echoix/grass

You can see that nothing in the gui folder is touched (it isn't even showing up yet).

Edit: when I mean touched, it's because I didn't limit yet to only tests or only tests that asserts. It also includes what is read when compiling (but partially, C code probably yes, but I don't think I ended up including Python in that set)

@landam landam assigned echoix and unassigned petrasovaa Jan 28, 2024
@echoix echoix merged commit 5b2460a into OSGeo:main Jan 28, 2024
23 checks passed
@echoix echoix deleted the fix-python2-builtin branch January 28, 2024 12:34
@echoix echoix removed the docs label Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI wxGUI related libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Uses of Python2-only import __builtin__ throughout the code base
6 participants