-
Notifications
You must be signed in to change notification settings - Fork 270
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
Added support for multiple API versions #533
Conversation
Note that no changes were made to the api, the code was refactored to allow for new versions of the api to be created down the road. Here's what this would look like: +-- api/ +-- v1/ +-- __init__.py +-- resources.py +-- v1_1/ +-- __init__.py +-- resources.py +-- v2/ +-- __init__.py +-- resources.py +-- __init__.py +-- common.py
/Users/drthrash/PycharmProjects/ihatemoney/ihatemoney/api/v1/resources.py reformatted /Users/drthrash/PycharmProjects/ihatemoney/ihatemoney/api/common.py All done! ✨ 🍰 ✨
All done! ✨ 🍰 ✨ 1 file reformatted, 22 files left unchanged.
Thanks for proposing this. I don't understand why this is required, though. Do you expect IHM to handle multiple API version at the same time? I mean. This change seems meaningful only if we are to propose a different API version, right? |
Yes, it will allow more freedom for developers. There won't have to be community consensus to add/modify an api endpoint. And yes, I have an optional feature that I'd like to implement and it won't use the default "/api/" blueprint. |
Are we good to merge this now? |
Hi @DavidRThrashJr, it's still not clear to me why we need this, sorry ! From what I understand, the changes you proposed only move some files around. The API is still reachable at the same place than it was. I don't understand why the files needs to be moved to a different location, and why it would allow for more flexibility. Please, can you explain better why you think this is necessary? Thank you :-) |
The current state of things seems to allow developers to add optional features using a different endpoint. |
Having this structure is best practice. Right now api.py is in the main project folder. To add another api endpoint you would need to define multiple api endpoints in the same file, which would decrease readability. Another option is to create a new file perhaps apiv2.py, this file would be in the main project folder, thus making the project layout not as clean. |
I agree on this code split. I don't see a useful case for it in this application, but this is not a valid reason: let's create the ability for other people to have nice idea. For example, it can be a good architecture for some creative developers to test new API, before getting integrating into the default API. So for me: let's go 👍 |
As you're two people willing to get this into core I've merged. Thanks for the feedback! I hope this message won't offend anyone, I really just want to understand what I'm missing, hence the rest of the message. This is (obviously) not blocking, but I still don't understand why it can allow more "[…] developers to test new API, before getting integrated into the default API.", I might be missing something here : to me, what I see here only adds a layer of complexity (multiple files and folders). To me, "premature optimization is the root of all evil", and as such, I see this as a "premature optimization", as I don't see what it actually brings to the table. I agree with you that if something makes it easier to developers to play and get creative, then it's a good thing, but here I don't understand how this change makes things easier for devs. Feel free to ignore me if you think I'm pushing it too much ;-) |
You may be right. I really don't know if such an architecture will be useful. But I know that not having it can cause troubles if something change in the API, and that clients relies on it. This is still a small project, so breaking the API is not so much an issue, but still, it is good practice to have a version number in it, so that old and new clients can use the same backend with 2 API. Of course, this change could have been introduced when it is really needed. But as @DavidRThrashJr said:
And in the end, this PR stays relatively low on file changes. It introduces a two directories with 2 files (I skip |
I completely agree with this, but that's not what this pull request does : it actually doesn't add a version number for the clients: the URL stays the same, and the version number is not anounced, so it clearly doesn't change anything for clients, unless I missed something, this is purely aesthetics on how the files are organised. If we were to support a versioning scheme, then I believe we should follow the kinto standards : an API version number and an announce of the capacities. Thanks for your inputs :-) |
Can you provide a link for more information? I definitely would like to follow a standard, when developing the new api version. |
Sorry, I should have put this in the first place. Here is a link to versioning info : https://docs.kinto-storage.org/en/stable/api/versioning.html Kinto docs are useful in general because they have already planned on how to do things like syncing, filtering, paginating, etc. |
* Added support for multiple API versions Note that no changes were made to the api, the code was refactored to allow for new versions of the api to be created down the road. Here's what this would look like: +-- api/ +-- v1/ +-- __init__.py +-- resources.py +-- v1_1/ +-- __init__.py +-- resources.py +-- v2/ +-- __init__.py +-- resources.py +-- __init__.py +-- common.py * reformatted using black /Users/drthrash/PycharmProjects/ihatemoney/ihatemoney/api/v1/resources.py reformatted /Users/drthrash/PycharmProjects/ihatemoney/ihatemoney/api/common.py All done! ✨ 🍰 ✨ * Applying fix for unused import in init.py https://stackoverflow.com/questions/31079047/python-pep8-class-in-init-imported-but-not-used * Formatting changes recommended by black All done! ✨ 🍰 ✨ 1 file reformatted, 22 files left unchanged.
* Added support for multiple API versions Note that no changes were made to the api, the code was refactored to allow for new versions of the api to be created down the road. Here's what this would look like: +-- api/ +-- v1/ +-- __init__.py +-- resources.py +-- v1_1/ +-- __init__.py +-- resources.py +-- v2/ +-- __init__.py +-- resources.py +-- __init__.py +-- common.py * reformatted using black /Users/drthrash/PycharmProjects/ihatemoney/ihatemoney/api/v1/resources.py reformatted /Users/drthrash/PycharmProjects/ihatemoney/ihatemoney/api/common.py All done! ✨ 🍰 ✨ * Applying fix for unused import in init.py https://stackoverflow.com/questions/31079047/python-pep8-class-in-init-imported-but-not-used * Formatting changes recommended by black All done! ✨ 🍰 ✨ 1 file reformatted, 22 files left unchanged.
Note that no changes were made to the api, the code was refactored to allow for new versions of the api to be created down the road.
Here's what this would look like:
+-- api/
+-- v1/
+-- init.py
+-- resources.py
+-- v1_1/
+-- init.py
+-- resources.py
+-- v2/
+-- init.py
+-- resources.py
+-- init.py
+-- common.py