-
Notifications
You must be signed in to change notification settings - Fork 0
Create api definition activies#4 #8
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
Conversation
5145117 to
3716435
Compare
time_tracker_api/__init__.py
Outdated
| return flask_app | ||
|
|
||
|
|
||
| def init_app(app): |
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.
Could we use type hints here https://docs.python.org/3/library/typing.html ?
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.
Makes sense, I will do it.
| description="API for the TimeTracker project") | ||
|
|
||
| # APIs | ||
| from time_tracker_api.projects import projects_api |
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.
Shouldn't we put all the imports at the top?
On the other hand, the import from time_tracker_api.projects import projects_api from what I understand, you import the file projects_api and use it to reference functions inside of it. But It took me some minutes to figure it out. I think it would more easy to grasp something like from time_tracker_api.projects.projects_api import ns as projects_ns. But feel free to argument your approach.
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.
The thing is that when I see the variable projects_ns it looks like an alias but it is not clear where it comes from, also it makes that import line bigger. When I see projects_api.ns, I understand that it comes from projects_api and that the variable it references is ns, there is no possible abstract interpretation.
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 did the import in the same part where I added that namespace because it seemed a good idea to have them together in case I wanted to remove or comment the inclusion of a particular namespace.
3716435 to
6106970
Compare
6106970 to
d9bb604
Compare
This PR creates an API definition to manage activities.