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

GTC-3172: Support GADM areas in drivers endpoints #656

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

gtempus
Copy link
Contributor

@gtempus gtempus commented Mar 12, 2025

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Check the commit's or even all commits' message styles matches our requested structure.
  • Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

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):

{
  "aoi": {
    "type": "admin",
    "country": "BRA",
    "region": "12",
    "subregion": "23"
  }
}

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?

  • Yes
  • No

@gtempus gtempus changed the title 📝 docs(DataMart): update docs to enable Swagger usage GTC-3172: Support GADM areas in drivers endpoints Mar 12, 2025
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 40.20619% with 58 lines in your changes missing coverage. Please review.

Project coverage is 77.29%. Comparing base (0e6cc35) to head (7cd6d1b).

Files with missing lines Patch % Lines
app/routes/datamart/land.py 17.24% 24 Missing ⚠️
app/crud/geostore.py 21.42% 22 Missing ⚠️
app/models/pydantic/datamart.py 69.23% 12 Missing ⚠️

❗ 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     
Flag Coverage Δ
unittests 77.29% <40.20%> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gtempus gtempus force-pushed the gtc-3172/add-gadm-aoi branch from 763c65a to 0a85d07 Compare March 14, 2025 15:36
@gtempus gtempus force-pushed the gtc-3172/add-gadm-aoi branch from 3c582c5 to 3ee4a72 Compare March 18, 2025 15:58
@gtempus gtempus marked this pull request as ready for review March 18, 2025 16:27
@dmannarino
Copy link
Member

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):
Copy link
Member

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

Copy link
Contributor Author

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

@gtempus
Copy link
Contributor Author

gtempus commented Mar 21, 2025

@dmannarino, I ran the pre-commit hook in this commit: 6193fc5

@gtempus gtempus requested a review from jterry64 March 21, 2025 19:58
Copy link
Collaborator

@danscales danscales left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants