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

[Bug] Should grass.script expose ScriptError from wildcard imports? #4838

Closed
echoix opened this issue Dec 14, 2024 · 9 comments · Fixed by #5045
Closed

[Bug] Should grass.script expose ScriptError from wildcard imports? #4838

echoix opened this issue Dec 14, 2024 · 9 comments · Fixed by #5045
Labels
bug Something isn't working Python Related code is in Python

Comments

@echoix
Copy link
Member

echoix commented Dec 14, 2024

Describe the bug

https://github.com/OSGeo/grass/blob/3b5184f3454c69afdf2844479403a5d8ef5d24e0/python/grass/script/__init__.py now lists the names for wildcard imports, in the __all__ variable.
The text ScriptError is found 55 times. In some calls in gui, it is used like

  • gs.ScriptError multiple times
  • gcore.ScriptError in gui/wxpython/animation/dialogs.py
  • grass.ScriptError in gui/wxpython/gui_core/forms.py, where grass is from grass.script import core as grass
  • ScriptError in gui/wxpython/core/toolboxes.py and more, where from grass.exceptions import ScriptError, CalledModuleError is used

What should be done? Technically, we show that ScriptError isn't listed from grass.script in the init file.

@echoix echoix added bug Something isn't working Python Related code is in Python labels Dec 14, 2024
@echoix
Copy link
Member Author

echoix commented Jan 28, 2025

@petrasovaa Maybe you'd have an opinion on this

@petrasovaa
Copy link
Contributor

I think from grass.exceptions import ScriptError is the correct way, no?

@echoix
Copy link
Member Author

echoix commented Jan 28, 2025

I think from grass.exceptions import ScriptError is the correct way, no?

That's what I used in #4841. I also think it should be in the GUI code too. It might also help in reducing the circular imports noticed by Pylint 3 (but disabled for now in order to be able to upgrade).

@echoix
Copy link
Member Author

echoix commented Jan 28, 2025

And for the usages of ScriptError imported from grass.script.core (found in some GUI code), should they be changed to the correct import? As currently, they rely on a side effect of having ScriptError imported inside grass.script.core (for grass.script.core's purposes, but not declared)

@petrasovaa
Copy link
Contributor

And for the usages of ScriptError imported from grass.script.core (found in some GUI code), should they be changed to the correct import? As currently, they rely on a side effect of having ScriptError imported inside grass.script.core (for grass.script.core's purposes, but not declared)

Yes I think so.

@wenzeslaus
Copy link
Member

I'm pretty sure the variations are from before creation of grass.exceptions. We should import from where it is defined.

@arohanajit can you please make the changes? @echoix let us know if you are already working on it.

@echoix
Copy link
Member Author

echoix commented Feb 1, 2025

No I didn't work on it, but since the decision is made, it's quite straightforward to do

@arohanajit
Copy link
Contributor

I'm pretty sure the variations are from before creation of grass.exceptions. We should import from where it is defined.

@arohanajit can you please make the changes? @echoix let us know if you are already working on it.

I'll standardize across all the files.

@echoix
Copy link
Member Author

echoix commented Feb 4, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Python Related code is in Python
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants