-
Notifications
You must be signed in to change notification settings - Fork 25
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
Streamlined REST API Methods then using the default endpoint generator #67
Comments
To be honest, I thought passing an empty string was working. Thanks for all the issues, by the way! Really great that you caught all of this stuff. About the standard one, I don't have a formed opinion. I think if more people would comment here in favor, maybe we could change it to the cleaner option. |
I had similar thoughts to @waza-ari, I think such simplification would be nice. Maybe we could even unify paginated and not paginated endpoints by introducing some optional pagination param, which makes it return all records if None or something like that. |
This pagination part has been dealt with in #62, probably not the best option to make unpaginated a default endpoint (people might just add it blindly and then all of a sudden you allow users to get all your data at once). I think we could simplify the standard one though, go for the cleaner version. Any of you guys want to do it? |
Before saying yes, my question would be how to handle this change. If we change the default endpoint names and go for the "cleaner" version, that has the potential of breaking the code for other users relying on the current default behaviour. What would be your idea around that? |
We add a warning in the release and in the docs and change version from 0.11.x to 0.12.0 to indicate breaking changes. Since we're at 0.y.z, breaking changes are acceptable (we're still figuring things out). We could also add a warning message when using EndpointCreator for the next couple of releases to indicate that things changed. |
@waza-ari would be great if you could help with this! I would go for GET PATCH and DELETE We can think about the warnings together once you issue a PR. |
@waza-ari do you still plan to do this? |
Is your feature request related to a problem? Please describe.
First of all, thanks for your work and sorry for opening a million issues. This one is more of a debate I assume, as there is no "right" way of doing it. I do have the feeling that the current standard set of endpoints I suboptimal. Basically using the example:
Yields these endpoints:
One example is:
DELETE /api/v1/abilities/delete/{id}
- we already know its deleting because of the HTTP verb - why does it need to be in the path again? The same applies tocreate
(POST),update
(PATCH) andget
(GET) as well.Describe the solution you'd like
I'd love to be able to configure something "cleaner" (although I'm fully aware that's very opinionated) like this:
The HTTP Verb here together with the endpoint is quite descriptive in itself (and obviously I should change the default endpoint description, but its just for the demo).
Its not possible to achieve this with customised endpoint names either, as it would yield a double slash even if setting the endpoint name for
delete
to an empty string.The text was updated successfully, but these errors were encountered: