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 4 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.
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 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.
Consider throwing an exception 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.
I cleaned this because I plan to give tesk a command line tool like accessiblity, so app.py file would contain those logic this would be good as tesk would mostly be used in pods and interacted via commands.
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.
did not get it, can you like some sort of document relevant to this
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 think it would be good to use command pattern here, note that the tesk would be run in containers/pods and it should be make sense that it follows a standard cli pattern. Current if you see
pyproject.toml
we have two console script for tesk, iefiler
andtaskamster
, which makes it look like they are two seperate binaries/scritps.I think all three component should be launched with an argument (upto discussion this is just an example) as that will make more sense, with #179.
What do you think @uniqueg @kushagra189 ??
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 think it makes sense, but would like to hear feedback also from @lvarin and/or @jemaltahir.
Besides that, I don't think that this change belongs in this PR, therefore I'm not reviewing details 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.
hope we have tested backward compatibility 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.
Nah :)
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 be raising appropriate runtime exception here.