[bhuvan-somisetty]

Description

In python/grass/pygrass/raster/__init__.py, RasterSegment.open() previously called fatal(str_err.format(self)) from grass.script when attempting to open an existing raster map in write ("w") or read-write ("rw") mode without overwrite=True.

This change:

• Replaces fatal() with raising OpenError(str_err.format(self.name)).
• Removes the unused from grass.script import fatal import.
• Adds unit tests in RasterSegmentTestCase within python/grass/pygrass/raster/testsuite/test_pygrass_raster.py covering OpenError on non-existent maps in read mode, existing maps in write/read-write modes without overwrite, and successful opens with overwrite=True.

Motivation and context

Calling fatal() inside PyGRASS library classes immediately terminates the Python process (sys.exit(1)), preventing applications, notebooks, and test runners from catching and handling the exception via except OpenError:. Raising OpenError aligns RasterSegment with RasterRow.open() and standard PyGRASS exception handling.

Fixes #8007.

How has this been tested?

Added unit tests in python/grass/pygrass/raster/testsuite/test_pygrass_raster.py (RasterSegmentTestCase) and verified with ruff check and ruff format.

Screenshots (if appropriate)

N/A

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as before)

Checklist

• PR title provides summary of the changes and starts with one of the pre-defined prefixes
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.

[bhuvan-somisetty]

Hi @ninsbl @echoix, I'm really enjoying contributing to GRASS GIS and would love to help out more across pygrass and other core tools. Please let me know if there are any specific issues or areas you'd recommend taking on next!

[echoix]

Try to use addClassCleanup and friends for handling incomplete setup teardown or errors in the setup class. I had already changed some gunittest test files with that (when it was causing problems on Windows), at least 3, maybe 5 or more.

I don't remember exactly what was the best way for the region, but I remember already handling that somewhere in the code.

[bhuvan-somisetty]

Done, switched to addClassCleanup for both the map and the region.

[echoix]

Can you refresh me on what self.name resolves to? Will it always resolve?

[bhuvan-somisetty]

It's the plain name from the constructor, so it's always set, but it drops the mapset. Now it uses self (name@mapset) like RasterRow.

[echoix]

Also consider these potential problems (below), judging if they are justified and correct issues or not.

If it is right about the part that RasterRow already has a check similar, then adapting the abstract base might be a better solution.

Details

I checked out PR #8008 as branch pr8008 (commit 1f46f67), which touches two files. GRASS isn't built in this checkout, so I couldn't run the new tests; the findings below come from reading the code only. The fix itself is right: fatal() becomes OpenError, and the is False to not change does nothing because the overwrite property only accepts True or False. The remaining issues sit in unchanged lines of RasterSegment.open(), the error message, and the new tests.

10 findings, most severe first:

1. python/grass/pygrass/raster/__init__.py:546: In the same function the PR fixes, opening an existing map in mode 'w' with overwrite=True but no mtype fails with a KeyError from RTYPE[None], where it should raise OpenError (RasterRow raises OpenError('Raster type not defined') here).

   Failure scenario: RasterSegment('elev').open(mode='w', overwrite=True) on an existing map: self.mtype is None, so RTYPE[None] raises KeyError. Callers who now write except OpenError (the error handling this PR sets up) miss it. The new tests always pass mtype='DCELL', so this path is never tested.

2. python/grass/pygrass/raster/__init__.py:554: Same problem for a map that does not exist: mode 'w' or 'rw' without mtype hits RTYPE[None] and raises KeyError instead of OpenError.

   Failure scenario: RasterSegment('new').open('w') raises KeyError: None instead of a clear 'Raster type not defined' OpenError, unlike RasterRow.open.

3. python/grass/pygrass/raster/__init__.py:522: The message argument changed from self (name@mapset) to self.name, so the error drops the mapset and no longer matches RasterRow.open, which formats with self.

   Failure scenario: Opening 'elev@PERMANENT' in mode 'w' without overwrite reports 'Raster map already exists', so you can't tell which mapset's map blocked the write. RasterRow reports elev@PERMANENT for the same situation.

4. python/grass/pygrass/raster/__init__.py:522: Behavior change for existing callers: they used to get gs.fatal(), which exits the process or, with set_raise_on_error(True), raises ScriptError. Now they get OpenError, which is not a ScriptError subclass.

   Failure scenario: A script that calls gs.set_raise_on_error(True) and wraps RasterSegment.open in except ScriptError no longer catches this error, and the commit message doesn't call out the API change.

5. python/grass/pygrass/raster/__init__.py:519: The overwrite check comes after info.read(), cats.read() and hist.read(), so all of an existing map's metadata is read just to raise an error, and the object is left with mode, overwrite and metadata changed after a failed open.

   Failure scenario: open('w') on an existing map without overwrite reads info, categories and history (which can be large) and then raises. The instance now keeps mode='w' and overwrite=False, so a retry with open() and no arguments fails again. Moving the mode/overwrite check to just after self.exist() avoids both.

6. python/grass/pygrass/raster/testsuite/test_pygrass_raster.py:127: test_open_w_overwrite opens the shared class map in mode 'w' and closes it without writing any cells, which replaces the fixture's contents with whatever was in the segment buffer. The tests now depend on the order they run in.

   Failure scenario: After test_open_w_overwrite runs, the fixture map no longer holds row() + 10*col(). Any value-based test added later to this class passes or fails depending on test order. The test should use its own map name, or rebuild the fixture in setUp.

7. python/grass/pygrass/raster/testsuite/test_pygrass_raster.py:116: The tests only check that OpenError is raised. They don't check that the existing map is left untouched or that no file descriptor or segment was opened, which is the actual guarantee the fix is meant to give.

   Failure scenario: A regression that calls Rast_open_new before raising would truncate the user's map while still raising OpenError, and every new test would still pass. Adding assertFalse(r.is_open()) plus a check of the map's values would catch it.

8. python/grass/pygrass/raster/testsuite/test_pygrass_raster.py:102: AGENTS.md convention: 'Use f-strings for non-translatable strings'. The new test builds a non-translatable expression with % formatting: expression="%s = row() + (10.0 * col())" % (cls.name).

   Failure scenario: This breaks the AGENTS.md rule in /home/user/grass/AGENTS.md (included from .claude/CLAUDE.md) and copies an outdated pattern into new code; the f-string form is f"{cls.name} = row() + (10.0 * col())".

9. python/grass/pygrass/raster/testsuite/test_pygrass_raster.py:96: setUpClass and tearDownClass are copied almost verbatim from RasterRowTestCase (same region, same mapcalc expression, same cleanup); only the map name differs.

   Failure scenario: Two copies of the fixture code have to be kept in sync. A shared base TestCase that takes name as a class attribute would remove the duplicate.

10. python/grass/pygrass/raster/__init__.py:520: The fix is local to RasterSegment.open: it re-implements the exists/overwrite check that RasterRow.open already has (with a different message), when a shared helper on RasterAbstractBase could do it for both classes.

    Failure scenario: RasterRow says 'already exists and will be not overwritten' while RasterSegment says 'already exists. Use overwrite.', and the next fix to this check has to be made twice. A single base-class method that raises OpenError would keep the two consistent.

[bhuvan-somisetty]

Went through the list. The check is now a shared helper on the base class and runs before the metadata reads. The overwrite tests use their own copies, and the failure tests check that the map is left untouched. 1 and 2 don't apply because mtype defaults to CELL, so it's never None.