Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grass.script: Make grass_path keyword only in init #3689

Merged
merged 1 commit into from
May 9, 2024

Conversation

wenzeslaus
Copy link
Member

@wenzeslaus wenzeslaus commented May 6, 2024

This makes the grass_path parameter a keyword-only argument (PEP 3102). While this technically breaks the 8.0 API for 8.4, the documentation only shows grass_path as a keyword argument, grass_path is required to be a keyword argument when location and mapset are omitted, and given the automation triggered by grass_path=None, any usage of grass_path is likely limited (the usage would have to be as positional argument to trigger the incompatibility).

API

Originally posted as a comment in #3438:

Worked in 8.0-8.3:

gs.setup.init("~/data/nc_spm/new_highway")
gs.setup.init("~/data/nc_spm/new_highway", grass_path="/opt/grass/bin/grass")
gs.setup.init("~/data", "nc_spm", "new_highway")
gs.setup.init("~/data", "nc_spm", "new_highway", "/opt/grass/bin/grass")

With this PR:

gs.setup.init("~/data/nc_spm/new_highway")
gs.setup.init("~/data/nc_spm/new_highway", grass_path="/opt/grass/bin/grass")
gs.setup.init("~/data", "nc_spm", "new_highway")
gs.setup.init("~/data/nc_spm/new_highway", env=env)
gs.setup.init("~/data/nc_spm/new_highway", grass_path="/opt/grass/bin/grass", env=env)
gs.setup.init("~/data", "nc_spm", "new_highway", env=env)

Never possible:

gs.setup.init("~/data/nc_spm/new_highway", "/opt/grass/bin/grass")

With this PR, code without an explicit grass_path working in 8.0-8.3 needs to be rewritten:

- gs.setup.init("~/data", "nc_spm", "new_highway", "/opt/grass/bin/grass")
+ gs.setup.init("~/data", "nc_spm", "new_highway", grass_path="/opt/grass/bin/grass")

Back to my earlier comment, the usage which is no longer available with this PR was never promoted. Instead, the documentation for grass.script.setup.init shows:

import grass.script as gs
session = gs.setup.init(
    "~/grassdata/nc_spm_08/user1",
    grass_path="/usr/lib/grass",
)

There is a heuristic in the function which determines the path automatically, so the idea is that in most cases, grass_path does not have to be supplied at all. (For example, I don't see any usages in this repo or on GitHub.) Having grass_path passed as a keyword argument (with explicit name), rather than a positional one, is the intended usage (based on the example, number and type of parameters and my judgement of what I intended back in 8.0 times).

One can say that this PR limits the usage to intended usage by fixing bug in the function specification.

@wenzeslaus wenzeslaus added this to the 8.4.0 milestone May 6, 2024
@wenzeslaus
Copy link
Member Author

To get the API update to 8.4, I'm creating a separate PR for the API spec change from #3438. The PR description is updated with an overview of the function usage.

@github-actions github-actions bot added Python Related code is in Python libraries labels May 6, 2024
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.

I agree like with the other PR. Does anyone else except us two want to jump in before merging?

This makes the grass_path parameter a keyword-only argument (PEP 3102). While this technically breaks the 8.0 API for 8.4, the documentation only shows grass_path as a keyword argument, grass_path is required to be a keyword argument when location and mapset are omitted, and given the automation triggered by grass_path=None, any usage of grass_path is likely limited (the usage would have to be as positional argument to trigger the incompatibility).
@echoix echoix force-pushed the make-grass_path-always-keyword branch from 75c291e to 4543585 Compare May 7, 2024 12:16
@wenzeslaus wenzeslaus merged commit 6805375 into OSGeo:main May 9, 2024
26 checks passed
@wenzeslaus wenzeslaus deleted the make-grass_path-always-keyword branch May 9, 2024 03:34
@wenzeslaus
Copy link
Member Author

Merged. This one would be easy to revert if needed.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants