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

wxGUI: Fix F405 error by explicitly importing required modules in iscatt/ #4426

Merged
merged 6 commits into from
Oct 2, 2024

Conversation

arohanajit
Copy link
Contributor

Fixed all F405 and F403 by explicitly importing all the functions. I had to import ctypes separately as well since explicit import of ctypes.c_void_p wasn't working for some reason. Also updated ctypes.POINTER to POINTER since its being imported.

petrasovaa
petrasovaa previously approved these changes Oct 1, 2024
@github-actions github-actions bot added GUI wxGUI related Python Related code is in Python labels Oct 1, 2024
@petrasovaa petrasovaa added this to the 8.5.0 milestone Oct 1, 2024
Copy link
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're at it, would you try to have these new imports following the order of what the tool "isort" would do? But please only keep the changes on like new lines, not reordering existing ones too much.

I already tried to change everywhere, but I didn't manage to have a perfectly tuned config. And made hard to read diffs. But we are still tending towards this.

Using isort the/file/path.py, without any config, does a correct enough job to the new lines+their placement, and will greatly reduce the diff of when we will be applying isort everywhere (I hope in the 1-1.5 year timeframe)

@echoix
Copy link
Member

echoix commented Oct 1, 2024

Remember to run black too after choosing the lines to commit.

@arohanajit
Copy link
Contributor Author

I ran isort which sorted the imports and subsequently the black which again rearranged them😅. Do you want me to do this for all the files I modify in future or only the ones where I work on F405?

@arohanajit arohanajit requested a review from echoix October 1, 2024 23:44
@echoix
Copy link
Member

echoix commented Oct 2, 2024

I ran isort which sorted the imports and subsequently the black which again rearranged them😅. Do you want me to do this for all the files I modify in future or only the ones where I work on F405?

When adding new imports, or when changing from * to explicit imports (so that sub list is sorted).

For this PR, there were two "unexpected" changes where imports not related to this PR got changed. But since they are simple enough, let's just keep it. These changes were the import numpy as np that got replaced with the from ctypes import , and the from grass.lib.imagery import * that got moved down.

Keep it like this, it's clean that way :)

@echoix echoix enabled auto-merge (squash) October 2, 2024 17:15
@echoix echoix merged commit 1b639db into OSGeo:main Oct 2, 2024
28 checks passed
@arohanajit arohanajit deleted the 405-core_c branch October 2, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI wxGUI related Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants