Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add base classes and docs #191
feat: add base classes and docs #191
Changes from all commits
d1ace7c
b879d6b
408c371
1bf8866
e232b52
540a5c0
0c8d352
bf33bc8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and other trivial changes may be required by linters, but they do not really fit in this PR. Open another
style:
PR to address these issues in bulk.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: Include in a
style:
PRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: Include in a
style:
PRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have we altered this? I see the parent model is still tesk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
segregation in the sidebar of documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference between submodules and subpackages? Any reason why we have used subpackages here and submodules at all other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autogenerated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the docs are auto-generated - why do we version control them, then? Does this require you to build the docs before committing (and if so, what if you forget)? Couldn't you just auto-generate them as part of your docs deployment and leave them out of the CI? For packages I wrote and documented their APIs via Sphinx and RtD, I only kept the
index.rst
and configuration - the pages were then generated by RtD and were not under version control (see FOCA, for example).Admittedly, the API docs I generated with Sphinx and RtD are pretty shitty 🤣 Still, do we really need these files here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not include
tesk.api.ga4gh.models
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autogenerated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember we fixed a few things here? Can you list the issues faced here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from foca.config.config_parser import ConfigParser
This throws:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is an issue that should be solved in a different PR though. Or were these errors a result of code added in this PR exclusively? If so, keep it here.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that all of those type packages should rather be added to FOCA, but I guess it's okay for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you are aware that Connexion validates requests and responses based on the OpenAPI schema without us having to write models? It's basically the main reason we use Connexion.
Admittedly, this predates the time of Pydantic - and I do like a good Pydantic model. However, this all may really not be necessary.
On another note, apart from your doc strings, I don't see anything TES-specific in here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we don't just use Pydantic's tools for writing validators? What are your concrete use cases here?
Also, note that this may change for the next FOCA version, which will use Pydantic v2 - and validators work quite differently from v1.
Again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need generic here? ABC should be sufficient IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to give type annotations to the value of the field I am getting, because the validator in future can be for a complex datatype as well, in that case it would be good that we can have type hinting of what data str will the methods return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have this enforced and throw exception by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do NOT change the OpenAPI file as it's an exact copy of the specs at the indicated commit hash, and we don't want to change it even for style reasons.