-
Notifications
You must be signed in to change notification settings - Fork 165
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
Add type hints to base and authentication
#472
Conversation
Hi @Viicos - thanks for raising these PRs I've been away and am still catching up, so will take a look at them next week 👍 |
Thanks @Viicos - this PR looks good - just need to fix the build
I should have done it when I did the last major, but I missed it - will have to wait until the next major
Sure, but I'll need to look at this in the next major |
Understandable as this would be a breaking change. I'll continue to add some type hints while still being careful not to overwhelm the code base (I'll rebase this one to get the CI fix from master) |
Hi @Viicos - is adding type hints a breaking change, I thought since the project was now >=3.7 this could be introduced in a non breaking way? |
Hi, sorry I was replying to your answer regarding the second point I asked in this PR description:
Switching to properties would be a breaking change as using |
I think I'll probably make another PR regarding the
|
I still have a few
I'll let you decide on these three cases what should be done (e.g. removing the default |
Thanks for spotting those @Viicos! You can make them all required arguments 👍 |
Had to ignore a few |
@adamjmcgrath I think this one is ready. I have a failing test on 3.10 due to this error on docs:
Do you happen to know how this can be fixed? |
Hi @Viicos - it looks like you can ignore this warning in the Sphinx config https://stackoverflow.com/a/30624034 |
Hi @adamjmcgrath, in case you did not get notified, this one is ready for review/merge |
Thanks @Viicos - I'll take a look this week |
lgtm, thanks @Viicos - can you just resolve that conflict and I'll approve and merge 👍 |
@adamjmcgrath If the last edit @b8a4a2c is good, I think this can be merged |
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.
Thanks @Viicos!
Fixes partially #470.
The goal is not to add complete type hints to the library, but only were it is beneficial. There are some code patterns that break mypy rules (e.g. Liskov substitution principle), and I'm not aiming at doing refactors, so once most of the type hints will be added, I'll write a mypy configuration accordingly.
I'm going for the
from __future import annotations
import, to improve type hints readability.Noticed a couple things:
This has been introduced two years ago, should it be deprecated now?
auth0-python/auth0/rest.py
Lines 108 to 111 in 44d6d0e
Could these methods be transformed into properties/class variables?
auth0-python/auth0/rest.py
Lines 113 to 127 in 44d6d0e