-
-
Notifications
You must be signed in to change notification settings - Fork 552
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
Type Annotate the *heck* out of social-core #986
Conversation
This enables typing in social_core and gives us the tools for testing
- ignore several errors that are endemic to mixin-type test strategies - in several cases assert that tests are set up correctly to give the type checker some help - for tidiness, mark some test-like class names as not-a-test
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #986 +/- ##
==========================================
+ Coverage 77.88% 77.93% +0.05%
==========================================
Files 347 347
Lines 10669 10698 +29
Branches 504 455 -49
==========================================
+ Hits 8310 8338 +28
Misses 2200 2200
- Partials 159 160 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5fc38b6
to
dd09461
Compare
dd09461
to
1c3e57a
Compare
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 please constantly use future typing with Type | None
syntax instead of mixing it with Optional[Type]
in some files?
1c87a55
to
7c92bb0
Compare
No... I just accidentally left in a test assertion. Fixing... |
Happy to; that was accidental in the early steps, where I forgot that we had Python new enough to use it. I'll revise ASAP. |
Merged, thanks for your contribution! |
This was a breaking change in API that social-app-django and other storages rely on.
This introduced API changes that break storage modules, #1020 reverts that part. |
This was a breaking change in API that social-app-django and other storages rely on.
This was a breaking change in API that social-app-django and other storages rely on.
This was a breaking change in API that social-app-django and other storages rely on.
This was a breaking change in API that social-app-django and other storages rely on.
Proposed changes
This is implementing the type checking I suggested in #949.
Types of changes
Please check the type of change your PR introduces:
Checklist
Put an
x
in the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.
Other information
This touches most of social-core, and entails some compromises due to overrides that pyright really does not like. These can be broken down into a few categories:
None
but is being treated as not, because it's set outside of the scope of the method. In these cases, I have addedassert <var>
statements to tell the type checker that the value is non-null; this code would have raised an exception at this point anywayget_user_id()
andget_user_details()
and the like are particularly egregious in this regard; I have added type:ignore comments in these cases.TODO
comments on these; I didn't try fix them here out of an abundance of caution, since I don't use those authentication providers.get()
methods, which actually led to type confusion, since sometimes more than one was mixed in. I renamedget()
onNonce
toget_nonce()
and onAssociation
toget_association()
in those cases.public_key = RSAAlgorithm.from_jwk(...)
is not returning a public key, it's returning an allowed keys type, at least with modern pyca-cryptography; it is probably wrong, but it's working. type:ignore'd