-
Notifications
You must be signed in to change notification settings - Fork 449
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
Added some type hints #354
Conversation
* run.py and config.py * app/database/models
1a14e38
to
015a220
Compare
@isabelcosta Could I get some feedback and answer to my questions? I'd really like to add those type hints :D |
You can submit the PR with that :) I haven't received any response either, and couldn't bring this to a Community Open Session either. Once I get the chance I'll ask about the community standards, where they stand. |
That's great! Thanks for a fast response :) |
d084303
to
801749e
Compare
@bartekpacia as I am catching up on PRs, I noticed this very important issue. I got more informed about this Python feature that I was not aware of and think it would be a great addition to the project. Since this has so many merge conflicts, would you like to fix them or leave this issue for other contributors? I was thinking of creating sub-issues per file or per feature or per domain, to make this change into type hints easier to manage and review. What do you think? I am totally fine if you choose not to continue with this and then I'll create smaller issue to make this transition. |
@isabelcosta I'll try to fix the conflicts in this PR. You can then open some issues requesting to add type hints to the rest of the project. |
Oki! Sounds good @bartekpacia , let me know once you fix it so I can review this :) And then I'll create type hint issues for what is missing. |
@isabelcosta Resolved conflicts :) |
thank you so much @bartekpacia |
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.
Very nice, thank you for this, learned something new 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.
Looks good :) Thank you so much for proposing this enhancement in the app and implementing part of it here in the PR @bartekpacia 🎉
* chore: added typings in * run.py and config.py * app/database/models * added type hints to app/api/dao
* chore: added typings in * run.py and config.py * app/database/models * added type hints to app/api/dao
Description
I added type hints to (files listed in commit message). I have a few questions though:
None
, should it also be typehinted?Also, I'm not sure how should I typehint objects returned by TasksListModel.find_by_id().
When a mentors accepts proposed changes and answers questions listed above, I'll be very happy to typehint whole codebase :)
Fixes #331 (as of now, partly)
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Type hints don't have runtime impact, but just to make sure I run all the tests. And CI is passing, too.
Checklist:
Code/Quality Assurance Only