Api shared datamodels #134
Conversation
liviuba
force-pushed
the
api_shared_datamodels
branch
from
f74dcc9 to
71d5c13
Compare
|
@liviuba I've got most of the docker compose changes here in the docker compose branch and I've rebased on this branch to merge them |
Thanks, will look at it! Do you want to merge into this before we merge this into main to avoid a re-rebase if we squash&merge this? If you want to merge into main, then I can fastforward main to this branch after the PR is done to merge, and then the docker-compose one shouldn't need another rebase |
|
Happy to it's just got some of your API code in it too now, so it won't be completely clean, but it's all additive so it won't cause obvious problems |
liviuba
force-pushed
the
api_shared_datamodels
branch
from
7965316 to
871304f
Compare
api/api/public.py
Outdated
| from .models.repository import Repository | ||
|
|
||
|
|
||
| router = APIRouter() |
There was a problem hiding this comment.
should the be using the public tag, like the private one is?
There was a problem hiding this comment.
Yep, I think I missed it, fixed
api/api/private.py
Outdated
| from fastapi import APIRouter | ||
| from pydantic.alias_generators import to_snake | ||
|
|
||
| # ? |
There was a problem hiding this comment.
Haha, must have added it as a note because I wasn't sure if importing directly would work out, deleted now
api/api/public.py
Outdated
| return get_item | ||
|
|
||
|
|
||
| for t in models_public: |
There was a problem hiding this comment.
am i right in thinking that this gets run when app.py imports the public and private endpoints? Just wondering whether it would be better python practice to have methods that get explicitly called in app.py...? (This is a question, not a suggestion, i'm genuinely not sure about the implications)
There was a problem hiding this comment.
Agree it would be nice to not run code in global scope at import, was just handwaving the problem until I get an example of a non-generated handler too, so I can see how I'd like to do it but still keep router global (to avoid nesting custom handlers in the factory), and make "the correct way" to use a router easy to tell (e.g. no import first and then a function to attach the generation functionality)
Added a fix, but I'm not sure we'll keep it like this once we have more functionality
There was a problem hiding this comment.
Generally looks good, but left a couple of comments. Checked it out and it builds locally with the docker command & was able to post & get a study (only got some errors due to the problems with model).
Was able to run the test 'manually' successfully but had issues with pydantic trying to find them - that's going to just be user error though.
* Rebase api on new main, new v2 wip module root * Add post handler, fix scoping * wip * wip rebase on remote branch & add wip work for actually saving data * Add date codec * Add first passing create/get test * Add all models * Speedup successive api builds * Fix GET * typing * Add public tag to public router * Delete ? * Add router factory
To test:
docker compose up -d --buildActual changes:
tests/test_minimal.py)api/private,api/public