grass.jupyter: Add utils tests and refactor to Tools API#7066
grass.jupyter: Add utils tests and refactor to Tools API#7066Rhushya wants to merge 1 commit intoOSGeo:mainfrom
Conversation
github-actions
bot
added
Python
There was a problem hiding this comment.
Pull request overview
This PR adds unit tests for GRASS Jupyter pure-Python utility helpers and refactors several GRASS-module-calling utilities in grass.jupyter.utils to use the newer grass.tools.Tools API.
Changes:
- Added
utils_test.pywith pytest coverage for pure-Python helpers (rendering size, command parsing, HTML formatting). - Refactored region and projection-related helpers in
utils.pyfromgrass.scriptrun_command/read_command/parse_commandtoTools. - Expanded
get_region_bounds_latlon()andupdate_region()to accept an optionalenvparameter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
python/grass/jupyter/utils.py |
Refactors multiple GRASS tool invocations to Tools, adds env support for region helpers. |
python/grass/jupyter/tests/utils_test.py |
Introduces pytest unit tests for pure-Python utility functions in grass.jupyter.utils. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| current = gs.region(env=env) | ||
| tools = Tools(env=env) | ||
| return tools.g_region( | ||
| flags="p" if gs.locn_is_latlong() else "pa", |
There was a problem hiding this comment.
update_region() accepts an env parameter and uses it for gs.region() and Tools(env=env), but gs.locn_is_latlong() is called without env. When env is provided for a different session/location, the chosen flags ("p" vs "pa") may be based on the wrong location. Pass env=env to gs.locn_is_latlong() for consistent behavior.
| flags="p" if gs.locn_is_latlong() else "pa", | |
| flags="p" if gs.locn_is_latlong(env=env) else "pa", |
| tools = Tools(env=env) | ||
| result = tools.r_proj( | ||
| flags="g", | ||
| input=raster, | ||
| mapset=mapset, | ||
| project=location, | ||
| dbase=dbase, | ||
| env=env, | ||
| ).strip() | ||
| params = gs.parse_key_val(output, vsep=" ") | ||
| output = gs.read_command("g.region", flags="ug", env=env, **params) | ||
| keyval = gs.parse_key_val(output, val_type=float) | ||
| cell_ns = (keyval["n"] - keyval["s"]) / keyval["rows"] | ||
| cell_ew = (keyval["e"] - keyval["w"]) / keyval["cols"] | ||
| ) | ||
| params = gs.parse_key_val(result.text.strip(), vsep=" ") | ||
| region = tools.g_region(flags="u", format="json", **params).json | ||
| cell_ns = (region["n"] - region["s"]) / region["rows"] | ||
| cell_ew = (region["e"] - region["w"]) / region["cols"] | ||
| return (cell_ew + cell_ns) / 2.0 |
There was a problem hiding this comment.
The refactor of estimate_resolution() to the Tools API isn’t covered by tests in python/grass/jupyter/tests (no references found). Adding an integration test (using the existing session fixtures) would help catch regressions in the r.proj/g.region interaction and the expected output parsing.
| cell_ew = (keyval["e"] - keyval["w"]) / keyval["cols"] | ||
| ) | ||
| params = gs.parse_key_val(result.text.strip(), vsep=" ") | ||
| region = tools.g_region(flags="u", format="json", **params).json |
There was a problem hiding this comment.
In estimate_resolution(), tools.g_region(flags="u", format="json", **params) is likely to produce no stdout (g.region without a printing flag), so Tools will return None and accessing .json will fail at runtime. Add a flag which produces output (e.g., include "p" or "g"), or switch to parsing key-value output (e.g., use .keyval) while preserving the previous behavior of g.region -ug.
| region = tools.g_region(flags="u", format="json", **params).json | |
| region = tools.g_region(flags="ug", format="json", **params).json |
| import pytest | ||
|
|
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
|
At least some of the copilot comments seem relevant, please get those addressed first. |
|
Looking at this more closely, the tests cover different parts of code, not what was changed. The migration is very basic/naive. Candidate for closing. |
2 participants
@petrasovaa @wenzeslaus @cwhite911
Description
This PR adds test coverage for
grass.jupyter.utilsand refactors several functions to use the new Tools API.Changes:
gs.run_command/gs.read_commandto the newToolsAPIAdded Stuff