-
Notifications
You must be signed in to change notification settings - Fork 5
Add all the latest from the Felt REST API #22
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
Conversation
7574284 to
c796e18
Compare
0ada2c5 to
dd41020
Compare
60b8681 to
f336b3a
Compare
arredond
left a comment
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.
Love the direction of this! Keeping the library up to date with the Felt API and adding tests is really invaluable. Left some small notes (mostly nits).
felt_python/elements.py
Outdated
|
|
||
|
|
||
| def list_elements_in_group( | ||
| def show_element_group( |
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.
Is "show" the right term here? This still returns a list, it doesn't print anything. The developer docs also say "list" so we should keep these consistent
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.
Yeah, show isn't correct here. To match all the other endpoint functions, this would be get_element_group.
The old list_elements_in_group makes sense when you read it, but doesn't follow the naming pattern. For example, we have get_layer_group not list_layers_in_group.
Also it technically returns a GeoJSON object, not a literal list.
I'll change it to get_element_group for now and we can consider if we should keep that or just stick with the old function.
This PR updates the
felt_pythonlibrary with all the latest capabilities from the Felt REST APIAlso closes #21