Skip to content

utils: migrate set_target_region to Tools API#7207

Open
AkshadSonawane wants to merge 2 commits intoOSGeo:mainfrom
AkshadSonawane:refactor/utils-tools-api
Open

utils: migrate set_target_region to Tools API#7207
AkshadSonawane wants to merge 2 commits intoOSGeo:mainfrom
AkshadSonawane:refactor/utils-tools-api

Conversation

@AkshadSonawane
Copy link
Copy Markdown

Continuing the Tools API migration from #6950, which covered get_region and get_location_proj_string. Two more functions in utils.py still used the old gs.* style — this cleans those up.
set_target_region now uses Tools(env=tgt_env).g_region(...) instead of gs.run_command. get_region_bounds_latlon switches to Tools().g_region(flags="bp", format="json").json — the -g flag is dropped since it was only needed for shell-script output formatting, which format="json" makes redundant.
I left update_region alone for now. Its return value gets unpacked by _update_output in interactivemap.py using long key names (north, south, etc.), and I wasn't confident the Tools API JSON output uses the same keys without a live environment to test against. Happy to revisit that one if there's a clean way to verify it.

@github-actions github-actions bot added Python Related code is in Python libraries notebook labels Mar 21, 2026
@AkshadSonawane AkshadSonawane changed the title refactor: migrate remaining utils functions to Tools API utils: migrate remaining functions to Tools API Mar 21, 2026
@echoix
Copy link
Copy Markdown
Member

echoix commented Mar 21, 2026

It's probably not a problem in this file, but we just have to watch out that our low-level library functions don't depend on high level functions, creating a circular dependency, which could cause import problems.

Copy link
Copy Markdown
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

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

Please test this locally.

@AkshadSonawane AkshadSonawane force-pushed the refactor/utils-tools-api branch from 91acd77 to 67e0180 Compare March 22, 2026 16:17
@AkshadSonawane AkshadSonawane changed the title utils: migrate remaining functions to Tools API utils: migrate set_target_region to Tools API Mar 22, 2026
@AkshadSonawane
Copy link
Copy Markdown
Author

It's probably not a problem in this file, but we just have to watch out that our low-level library functions don't depend on high level functions, creating a circular dependency, which could cause import problems.

Good catch — checked and Tools comes from grass.tools which sits below grass.jupyter in the dependency chain, so no circular dependency here. Good thing to keep in mind for future migrations though.

@AkshadSonawane
Copy link
Copy Markdown
Author

Please test this locally.

Tested locally. The screenshot shows Tools(env=tgt_env).g_region(...) — the exact pattern used in set_target_region — working correctly against a compiled GRASS 8.6.0dev build. Region values are set and verified via gs.region().
Also reverted get_region_bounds_latlon — the Tools API JSON output doesn't include the ll_* keys that function relies on, so that migration isn't safe without a geographic projection to test against. Updated the PR title to reflect that only set_target_region is being migrated.
Screenshot 2026-03-22 214025

@AkshadSonawane AkshadSonawane force-pushed the refactor/utils-tools-api branch 2 times, most recently from 983f1a2 to 67e0180 Compare March 26, 2026 21:12
@github-actions github-actions bot added the tests Related to Test Suite label Mar 27, 2026
@AkshadSonawane
Copy link
Copy Markdown
Author

Hi @petrasovaa,

I added a basic sanity test to ensure set_target_region is callable after the Tools API migration.

While attempting to write a full integration test using session_with_data, I ran into some ambiguity around the expected function signature and environment handling (especially regarding source vs. target environment usage).

Would you recommend a preferred testing pattern for this function, or should it be covered at a higher integration level? I’d be happy to expand the test coverage based on your guidance.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants