Skip to content

grass.script: Scan GRASS_ADDON_BASE in get_commands()#7031

Open
saket0187 wants to merge 5 commits intoOSGeo:mainfrom
saket0187:get-installed-extensions
Open

grass.script: Scan GRASS_ADDON_BASE in get_commands()#7031
saket0187 wants to merge 5 commits intoOSGeo:mainfrom
saket0187:get-installed-extensions

Conversation

@saket0187
Copy link
Contributor

This PR addresses issue mentioned in #3885

I have tested and verified that the output now includes installed extensions in the returned set.

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.

This looks good, but the user's expectation would probably be also looking at GRASS_ADDON_PATH here.

https://grass.osgeo.org/grass-devel/manuals/variables.html

@github-actions github-actions bot added Python Related code is in Python libraries labels Feb 3, 2026
@saket0187 saket0187 requested a review from wenzeslaus February 3, 2026 06:13
@wenzeslaus
Copy link
Member

Given this is based on a feature request, there is no test coverage. There are some g.extension tests where you can add this as an additional step conforming that GRASS_ADDON_BASE actually works. For GRASS_ADDON_PATH, it seems like the best solution is to create pytest-based test and create couple scripts in different dirs.

@github-actions github-actions bot added module general tests Related to Test Suite labels Feb 5, 2026
@saket0187 saket0187 force-pushed the get-installed-extensions branch from 222b167 to 0e44f4a Compare February 6, 2026 09:33
@saket0187
Copy link
Contributor Author

I have added a few lines to scripts/g.extension/tests/g_extension_test.py. Since conftest.py already configures GRASS_ADDON_BASE, I thought using that setup would be enough. I have also added a test in python/grass/script/tests/grass_script_core_get_commands_test.py that creates dummy executables to confirm get_commands() correctly scans the paths specified in GRASS_ADDON_PATH.

Do let me know if anything is incorrect or if any changes are needed.

@wenzeslaus The Windows 2022 build is failing due to HTTP 404 errors, which appears unrelated to my changes; my local tests are passing successfully.

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.

The pytest has a single test, no? Rework it to yield session instead of tools.

@saket0187
Copy link
Contributor Author

Currently there's only one test file right now (excluding conftest.py), But I have simplified the fixture to yield session directly which is good to have if we add some tests later. I moved the Tools logic into the test itself; shouldn't make much difference performance-wise since it's just used once.

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.

This looks great now. Thank you.

We should merge only after the Windows CI is back since this actually needs the Windows check.

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

Labels

general libraries module 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