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

enhance folder structure & apply API versioning #7

Conversation

YousefAldabbas
Copy link
Contributor

@YousefAldabbas YousefAldabbas commented Jul 16, 2024

closes #6

@YousefAldabbas YousefAldabbas changed the title closes #6 enhance folder structure & apply API versioning Jul 16, 2024
@IbraheemTuffaha
Copy link
Owner

Interesting!
Can you please fix the conflicts and push again?
This will also trigger the GitHub Actions, I have them enabled now for pull requests which should make them run on the PR

@YousefAldabbas YousefAldabbas force-pushed the #6-apply-api-versioning-and-enhance-folder-structure branch from bad46d2 to 4be3c6a Compare July 18, 2024 21:16
@YousefAldabbas
Copy link
Contributor Author

YousefAldabbas commented Jul 18, 2024

@IbraheemTuffaha please note i added the following lines to ignore flake errors

  ; Found local folder import
  WPS300,
  ; multiple statements on one line (colon)
  E701,
  ; Found incorrect node inside `class` body
  WPS604,

-WPS300: relative import is approved by Guido himself, i only use relative import in same folder files.
-E701: this violates black formatting so basically black formatting for class with no vars/methods/docs is class ClassName(Base): ... which raising this error
-WPS604: not sure about this error but most properly its pydantic issue (needs more investigation)

I believe for now we can merge this PR and open an issue for WPS604

@IbraheemTuffaha
Copy link
Owner

Do you know why there's still a conflict?

@YousefAldabbas
Copy link
Contributor Author

@IbraheemTuffaha hi, conflict resolved

@IbraheemTuffaha
Copy link
Owner

image For me it says there are still conflicts 👆

@YousefAldabbas
Copy link
Contributor Author

image

weird issue, i will investigate this issue more

@YousefAldabbas YousefAldabbas force-pushed the #6-apply-api-versioning-and-enhance-folder-structure branch 3 times, most recently from 1323452 to 420a27c Compare July 20, 2024 09:24
@YousefAldabbas
Copy link
Contributor Author

@IbraheemTuffaha is the conflict still exists?

@IbraheemTuffaha
Copy link
Owner

Thank you @YousefAldabbas for your contribution here, I just got the chance to review the changes..

I like the idea of versioning, although not all projects needs versioning, but sure let's put it here as a best practice..

It's also interesting how you put both requests & responses in serializers, I'm wondering.. what if I have multiple endpoints in dummy router?
Will that translate into multiple classes for each endpoint in the same app/v1/serializers/dummy.py file?

I've made a few changes to the code, let me know what you think:

  • Remove relative imports, I understand that it can be useful, I just prefer not to use them and keep things more explicit
  • For CapetalizeIn and CapitalizeOut, I've found from experience that inheriting just for renaming is confusing for the future, it complicates things and makes it harder to track, so let's just use Capitalize in this case, and maybe for other endpoints that needs different attributes for requests and responses, we can make multiple classes like you did
  • I chose not to use response_model, but actually use the return type as it's not needed here, usually you use response_model when you have to..

@IbraheemTuffaha IbraheemTuffaha force-pushed the #6-apply-api-versioning-and-enhance-folder-structure branch from 89c75ad to f475bc3 Compare July 28, 2024 17:53
@YousefAldabbas
Copy link
Contributor Author

Hi @IbraheemTuffaha, sorry for the late response.

"It's also interesting how you put both requests & responses in serializers. I'm wondering, what if I have multiple endpoints in a dummy router? Will that translate into multiple classes for each endpoint in the same app/v1/serializers/dummy.py file?"

APIs should return the same response schema for table CRUD operations. views/dummy.py APIs are going to have the same response since they serve the same purpose, which is handling x_table CRUD operations. For the get list response, the model is going to be List[serializer.DummyOut], and for the other operations, it will be serializer.DummyOut.

Of course, this depends on the purpose of your template. Is it for servers that only operate as gateways for other servers (or third parties) or is it going to handle DB operations and other logic

For the other points it's your repository and you can do whatever you want. no hard feelings it's just personal preference.
cheers

@IbraheemTuffaha
Copy link
Owner

No, I actually like your input.
I guess there are different types of routers/APIs, right?
Some of which do CRUD, and in this case you are right
Others do operations or processing and can yield completely different types of outputs
I think this is useful to make multiple sample routers, ones that do CRUD only
And others that are processing focused like the dummy we currently have
Let me play with this a little and see how it will look like

Copy link

sonarcloud bot commented Aug 18, 2024

@IbraheemTuffaha
Copy link
Owner

Reopened in #8

@IbraheemTuffaha
Copy link
Owner

IbraheemTuffaha commented Sep 2, 2024

It took a while 😅.. but now I've made some changes and pushed them
I couldn't do it on this PR for some reason, so I did it in #8

Please @YousefAldabbas take a look at main branch now and let me know what you think.. I've found your inputs here very helpful, so thank you!

I'll be working on adding another router that does operations and processing rather than CRUD, just as another example

@YousefAldabbas
Copy link
Contributor Author

Hi @IbraheemTuffaha,
I have three points / suggestions:
1- You placed the payload validators inside the model directory, which should contain ORM models instead. It would be better to place the serializers in a separate directory something like schema or serializers.

2- You created a user package inside the services folder that contains users_router.py, I don't really understand the purpose of that. Could you explain it further? Is this part of what the users' route can do or are you intending to add other features where the routes would start with https://domain/users ? If that's the case I don’t see any issue. Otherwise I think you should convert the users directory into a file and move the APIs from users_router.py into it.

3- I would move the code from base_router.py to routers/__init__.py I know this might be part of my old PR, but I was following the practice where you shouldn't have logic inside __init__.py files. but in this case we aren't introducing anything new except initializing the main router for the routers package.

if needed i will create a new branch contain all the changes that I'm thinking of.

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.

Apply Api Versioning & Enhance Folder Structure
2 participants