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

grass.pygrass: Improve exception handling in Info.open (#1555) #1559

Merged
merged 3 commits into from
Feb 28, 2022

Conversation

Viech
Copy link
Contributor

@Viech Viech commented Apr 30, 2021

No description provided.

python/grass/pygrass/vector/abstract.py Show resolved Hide resolved
python/grass/pygrass/vector/abstract.py Show resolved Hide resolved
python/grass/pygrass/vector/abstract.py Outdated Show resolved Hide resolved
@neteler neteler added enhancement New feature or request Python Related code is in Python labels Dec 9, 2021
@neteler neteler added this to the 8.0.1 milestone Dec 9, 2021
@ninsbl ninsbl modified the milestones: 8.0.1, 8.0.2 Feb 20, 2022
@ninsbl
Copy link
Member

ninsbl commented Feb 20, 2022

Bumping up milestone as 8.0.1 is due in two days, while this has not been part of RC1 and there has not been activity for some time.

@wenzeslaus wenzeslaus modified the milestones: 8.0.2, 8.2.0 Feb 27, 2022
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

After consulting Python documentation, I found one more issue here.

The rest of the code seems to be ready to be merged.

I merged main branch here to resolve conflict with new code on main (only a context line changed).

python/grass/pygrass/vector/table.py Show resolved Hide resolved
@wenzeslaus
Copy link
Member

All right, this can be merged as is. (CI is still running now.)

@Viech Two questions:

  1. You are using % formatting, but in this context .format() or f-string would be more appropriate with current Python. Any reason for that?

  2. Your error messages are not translatable (not marked with _(...), .format() or % are then needed, not f-string). Is that intentional? Python does not translate its error messages, but in GRASS we generally do, although for programming-only messages it is permissible not to translate (most of the new grass.jupyter does not have translated messages now). It would be good to discuss this on grass-dev and arrive at some best practice.

@Viech
Copy link
Contributor Author

Viech commented Feb 28, 2022

  1. You are using % formatting, but in this context .format() or f-string would be more appropriate with current Python. Any reason for that?

Likely because this MR is old and PyGRASS used that style at the time. I don't normally use %.

  1. Your error messages are not translatable (not marked with _(...), .format() or % are then needed, not f-string). Is that intentional? Python does not translate its error messages, but in GRASS we generally do, although for programming-only messages it is permissible not to translate (most of the new grass.jupyter does not have translated messages now). It would be good to discuss this on grass-dev and arrive at some best practice.

Same idea. Unless I missed something, I would've copied the style I found. Exception translation is something I don't think I've ever seen in any project, though. Users who can understand Python exceptions are very likely to understand English while users who see exceptions as gibberish will just post them in an issue and you'd end up with non-English exceptions on the tracker.

@wenzeslaus wenzeslaus merged commit d074fbd into OSGeo:main Feb 28, 2022
@wenzeslaus wenzeslaus changed the title PyGRASS: Improve exception handling in Info.open (#1555) grass.pygrass: Improve exception handling in Info.open (#1555) Apr 27, 2022
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
…o#1559)

* Add missing exception handling in Info.open to replace lower level and wrong exceptions.
* Add additional checks.
* Distinguish unsupported and unknown database drivers.

Fixes OSGeo#1555.
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
…o#1559)

* Add missing exception handling in Info.open to replace lower level and wrong exceptions.
* Add additional checks.
* Distinguish unsupported and unknown database drivers.

Fixes OSGeo#1555.
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
…o#1559)

* Add missing exception handling in Info.open to replace lower level and wrong exceptions.
* Add additional checks.
* Distinguish unsupported and unknown database drivers.

Fixes OSGeo#1555.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants