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

refactor: cleanup API and add pydantic models for typing #58

Merged
merged 16 commits into from
Aug 17, 2024

Conversation

b-rowan
Copy link
Collaborator

@b-rowan b-rowan commented Aug 8, 2024

General API and auth fixes, as well as pydantic models for most of the endpoints (except for download, because I moved it around in #47, and it cant use the models anyway). Moves things around to make it more clear what parts of the code do what.

@tsagadar
Copy link
Collaborator

tsagadar commented Aug 9, 2024

Why not splitting the code by operation? E.g. that we rather have a devices/delete.py that contains request, response and logic of the delete request? In the new org, one has to look at three files to understand a single request.

@b-rowan
Copy link
Collaborator Author

b-rowan commented Aug 9, 2024

Why not splitting the code by operation? E.g. that we rather have a devices/delete.py that contains request, response and logic of the delete request? In the new org, one has to look at three files to understand a single request.

I opted to mirror the way that FastAPI structures its directories. This is fine too, if we prefer that I can swap to that.

@tsagadar
Copy link
Collaborator

tsagadar commented Aug 9, 2024

Why not splitting the code by operation? E.g. that we rather have a devices/delete.py that contains request, response and logic of the delete request? In the new org, one has to look at three files to understand a single request.

I opted to mirror the way that FastAPI structures its directories. This is fine too, if we prefer that I can swap to that.

If this new way is documented somewhere then that is ok as well. On a quick search I found these practices which are close to the new structure: https://github.com/zhanymkanov/fastapi-best-practices?tab=readme-ov-file#project-structure

This would be another approach: https://stackoverflow.com/a/64987404

@UpstreamData UpstreamData force-pushed the dev_api_cleanup branch 7 times, most recently from a9fbe57 to 084846a Compare August 15, 2024 20:50
@b-rowan b-rowan requested a review from tsagadar August 15, 2024 20:50
Copy link
Collaborator

@tsagadar tsagadar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge rework - I like where this is going. One small thing left for this PR with the POST/PUT.

An open issue for another PR is then to remove the tenant and have the /ddi prefix.

Lastly: is the new file structure based on any documented approach how to organize such projects? If yes, reference it in the README.md. This could reduce discussions / uncertainties where to put what.

@b-rowan b-rowan requested a review from tsagadar August 16, 2024 14:52
@tsagadar tsagadar merged commit 4d70333 into master Aug 17, 2024
@tsagadar tsagadar deleted the dev_api_cleanup branch August 17, 2024 12:14
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.

4 participants