-
Notifications
You must be signed in to change notification settings - Fork 4
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
GTC-3172: Support GADM areas in drivers endpoints #656
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #656 +/- ##
===========================================
- Coverage 77.57% 77.29% -0.28%
===========================================
Files 142 143 +1
Lines 6528 6593 +65
===========================================
+ Hits 5064 5096 +32
- Misses 1464 1497 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
763c65a
to
0a85d07
Compare
First, make the change easy.
It used to grab the whole geostore and then just use the ID, which was wasteful.
And improve validation for `AdminAreaOfInterest`.
3c582c5
to
3ee4a72
Compare
In general this looks good! Holding off approving due to the simplification question. Also I think there's a little linting that went awry (either that or my knowledge of standards is out of date). Would you mind trying to run the pre-commit hooks on the involved files again? |
pass | ||
|
||
|
||
class GeostoreAreaOfInterest(AreaOfInterest): |
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.
One last comment: I feel CustomAreaOfInterest
or UserAreaOfInterest
may be more fitting name for the public facing doc
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 personally feel that those are too vague. But I'd like to hear what other folks think.
CC @jterry64 @dmannarino
@dmannarino, I ran the pre-commit hook in this commit: 6193fc5 |
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.
Just a number of minor comments. Looks good.
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Administrative boundary areas are not supported. The client must have a geostore ID
Issue Number: GTC-3172
What is the new behavior?
Clients can now use Administrative Boundaries with requests.
GET (query parameters):
aoi[type]=admin&aoi[country]=BRA&aoi[region]=12&aoi[subregion]=23
POST (body):
NOTE
There is now a grouping parameter (
aoi
) and a discriminator (aoi[type]
)The discriminator makes for very nice openapi documentation and allows much richer validation for pydantic unions
https://docs.pydantic.dev/latest/concepts/unions/#discriminated-unions
Only GADM 4.1 is currently supported
Does this introduce a breaking change?