Skip to content

Conversation

@wenzeslaus
Copy link
Member

@wenzeslaus wenzeslaus commented Sep 26, 2025

The create_project function (and the create_location function) specify in the documentation that it raises ScriptError on error. However, in some cases it calls fatal instead leading to system exit (SystemExit exception) instead with the default settings. This changes the fatal call into raise so that it aligns with the documentation and with other places in the function which raise ScriptError. This also makes it to behave in a more expected way in standard Python workflow.

Current GRASS code already wraps the calls into try-except with ScriptError. In case of GUI, it checks the specific existence condition ahead of time.

The exception text is not translatable according to the our new practice for exceptions.

This also removes the warning when the project exists and overwrite is set to True. This is explicitly requested behavior, so warning not required. Additionally, while it kind of fit together with fatal with overwrite set to False, not it does not fit. Removing it also removes the need for a translatable string at that place which is often used with minimal setup during startup, so the result is simpler, less fragile code.

Tests now cover all combinations cases (does not exist, exists, with and without overwrite).

The create_project function (and the create_location function) specify in the documentation that it raises ScriptError on error. However, in some cases it calls fatal instead leading to system exit (SystemExit exception) instead with the default settings. This changes the fatal call into raise so that it aligns with the documentation and with other places in the function which raise ScriptError. This also makes it to behave in a more expected way in standard Python workflow.

Current GRASS code already wraps the calls into try-except with ScriptError. In case of GUI, it checks the specific existence condition ahead of time.

The exception text is not translateable according to the our new practice for exceptions.

This also removes the warning when the project exists and overwrite is set to True. This is explicitly requested behavior, so warning not required. Additionally, while it kind of fit together with fatal with overwrite set to False, not it does not fit. Removing it also removes the need for a translatable string at that place which is often used with minimal setup during startup, so the result is simpler, less fragile code.

Tests now cover all combinations cases (does not exist, exists, with and without overwrite).
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.

Assuming tests and coverage results are adequate

@wenzeslaus wenzeslaus enabled auto-merge (squash) September 26, 2025 17:21
@wenzeslaus
Copy link
Member Author

Thanks for checking this. It is not just the new test, but there is existing similar test in place for overwrite, and then there is plenty of usage across the tests, although that does not check the error states explicitly.

@github-actions github-actions bot added Python Related code is in Python libraries tests Related to Test Suite labels Sep 26, 2025
@wenzeslaus wenzeslaus merged commit 40e99f6 into OSGeo:main Sep 26, 2025
27 of 28 checks passed
@wenzeslaus wenzeslaus deleted the raise-not-fatal-in-create_project branch September 26, 2025 19:30
@github-actions github-actions bot added this to the 8.5.0 milestone Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libraries Python Related code is in Python tests Related to Test Suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants