Proposal to split api.py into different files
Overview
This is a proposal of how api.py could be split up and is meant as a basis for discussion. The goal is to have a file structure like this:
pygeoapi/api/
├── __init__.py
└── coverages.py
└── features.py
└── processes.py
└── ...
This PR implements this for 2 endpoints of processes, but can be applied mechanically to all others.
There are also more possibilities for refactorings of the apis like extractions of common code or consolidation of error handling, but the idea here is to split up the file first, because it will necessarily be a huge diff, and smaller improvements can then be implemented in the single files at a later point.
One issue with the split is that API is a class and thus has to be in one file. This proposal changes the methods into regular functions which receive the API object as first parameter. Since we don't really use OOP features here, this is a simple change.
This commit performs the necessary adaption to transform an api function to the new style and shows that the changes are minor: https://github.com/geopython/pygeoapi/commit/78d6a0aef20cd67dd49ca91047b6be16af7d644d#diff-c9492e9767224068f9eb894b4558aea84f02c491d7a8975ae936f99ea7fc8232
Alternatives to this approach would be to either create a mixin in each file or to have classes for each api inheriting from a base API. I think both are more complicated but I'm happy to discuss
This proposal also includes an adapter from api functions to web framework endpoints (only implemented for flask now), which can replace decorators like pre_process and gzip, and which could be used in the future for a more convenient error handling or other abstractions: https://github.com/geopython/pygeoapi/commit/78d6a0aef20cd67dd49ca91047b6be16af7d644d#diff-631fe735514040ce51f87cfb0c8eeeb1b7895d42c9364cea597355062732c159R407
This proposal would also work without this adapter if we apply the decorators to all api functions as before.
Additionally to splitting up api.py, also the tests are moved to according files (https://github.com/geopython/pygeoapi/commit/f452d454d8736978889a21ca6b3c26d711a5dbb3) as well as the openapi defintion (https://github.com/geopython/pygeoapi/commit/b7d9201c9294acb05125415285cce3c19e9a3bb0)
Once we agree on a style for this whole refactoring, I can apply it to the whole api.py file.
Contributions and Licensing
(as per https://github.com/geopython/pygeoapi/blob/master/CONTRIBUTING.md#contributions-and-licensing)
- [X] I'd like to contribute [feature X|bugfix Y|docs|something else] to pygeoapi. I confirm that my contributions to pygeoapi will be compatible with the pygeoapi license guidelines at the time of contribution.
- [X] I have already previously agreed to the pygeoapi Contributions and Licensing Guidelines
I just rebased the branch. The tests currently fail due to a python version issue, which i can fix if we decide to actually follow this approach.
Strategy:
- split
pygeoapi/api.pyinto one file per API standard/specification, so:pygeoapi/api/__init__.pypygeoapi/api/processes.pypygeoapi/api/features.pypygeoapi/api/coverages.pypygeoapi/api/records.pypygeoapi/api/environmental_data_retrieval.pypygeoapi/api/maps.pypygeoapi/api/tiles.pypygeoapi/api/stac.py
pygeoapi/openapi.pywill be split into the above, at which pointpygeoapi/openapi.pywould import accordingly.
@totycro will begin with processes as the model to follow.
The above needs to work against existing tests for parity, at which point we can then refactor tests/test_api.py in the same manner.
Prior to starting this work, we will wait for the approve/merge of:
- #1313
- #1311
Updates:
- initial refactoring complete
- next steps
- refactor
tests/test_api.pyusing the same design pattern (per API) - ensure all tests pass
- perform manual testing (for UI)
- fix merge conflicts
- if not possible, we will send a note to the pygeoapi mailing list asking those who are
git cloned against pygeoapi master branch to hard reset
- if not possible, we will send a note to the pygeoapi mailing list asking those who are
- circulate for final review by pygeoapi devs
- refactor
- merge!
FYI this PR is now ready for review. I'd like to have this merged by Friday 05 April 2024, so review by that time would be valuable/appreciated.
nice work tom, agree with sander
nice work tom, agree with sander
To be fair, it was mostly not Tom's work ;) My mistake!
nice work tom, agree with sander
To be fair, it was mostly not Tom's work ;) My mistake!
Correct, @totycro deserves the credit here :)
Thanks for the feedback @ricardogsilva / @doublebyte1. I've tried to address most of the issues / comments in time for @totycro's Wednesday morning, as well as suggestion to defer other suggestions following the merge of this PR.
Note that there is an unrelated fix in https://github.com/geopython/pygeoapi/pull/1405/commits/eeb997c7207a052a140e02972f196f3b625fb0ab in support of passing the EDR tests based on possible data changes.
I think I addressed all of the comments by now. The way I see it, we don't have any issues really blocking the merge, and I'd prefer to merge it ASAP because many commits done on master right now will introduce new conflicts.