Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
library: Add imagery Python library module to grass.script #3756
base: main
Are you sure you want to change the base?
library: Add imagery Python library module to grass.script #3756
Changes from 15 commits
ec43c65
149c84a
a7764b1
3635ea8
f78b813
cc86c0b
f5aab58
e2b7880
88c2e25
2525777
85d36a0
9e7923c
569dff6
89c431d
25f1f97
1da985a
ebf46dd
0a6a697
e2965f0
a753a3b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean for it to be used this way, then don't add that to the
__init__.py
or leave.imagery
here out. (I don't know if it would even not work the way it is written now.) It is basically a decision between the imagery being part of the internal structure versus part of the API. Simpler imports versus more freedom in naming (no danger of collisions).I added
from . import setup
there so that there is no need for extra import, but that the namespace stays separate. Howeversetup
is a special case.For consistency with raster and others, having
from .imagery import *
seems appropriate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should tend to avoid adding new star imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, of course, but here in
__init__.py
and next tofrom .raster import *
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I included the star import is do have it consistent with the other modules...
And with that:
gs.imagery.group_to_dict(imagery_group)
does work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said,
gs.group_to_dict(imagery_group)
works too. I do what you prefer regarding the star-import. Not being able to dogs.group_to_dict(imagery_group)
without the star-import would be fine with me...Let me know what to do to merge it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... tests fail without the import of
imagery
in__init__.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it similar to https://python-notes.curiousefficiency.org/en/latest/python_concepts/import_traps.html#the-submodules-are-added-to-the-package-namespace-trap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or https://stackoverflow.com/a/40823467
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course the long way, import grass.script.imagery will work.
if you add it, then gs.imagery should work too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused. Why it works right now? I thought
from . import imagery
is needed forgs.imagery.group_to_dict
to work. I think I had to do it that way forfrom . import setup
andgs.setup.init
.