Skip to content
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

fix(mfa): Add types to allauth.mfa flows #3866

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Conversation

akx
Copy link
Contributor

@akx akx commented Jun 4, 2024

This PR adds a bunch of types to allauth.mfa's internals.

Our app needs to set up MFA with a slightly custom flow, so we can't use the forms or headless features, so I needed to delve a bit into how things work under the hood, and figured it'd be useful if types were documented technically too, for IDEs etc.

Feels like TOTP and RecoveryCodes should have a superclass, but that's beyond the scope of this PR...

@akx akx force-pushed the mfa-types branch 2 times, most recently from 2a59ebe to 7b16e50 Compare June 4, 2024 12:27
@coveralls
Copy link

coveralls commented Jun 4, 2024

Coverage Status

coverage: 95.749% (+0.001%) from 95.748%
when pulling c2465b2 on akx:mfa-types
into 46e4b51 on pennersr:main.

@akx akx force-pushed the mfa-types branch 2 times, most recently from 34569f5 to 7552540 Compare June 7, 2024 06:41
@akx akx marked this pull request as ready for review June 7, 2024 06:47
allauth/mfa/models.py Outdated Show resolved Hide resolved
allauth/mfa/recovery_codes.py Outdated Show resolved Hide resolved
@pennersr
Copy link
Owner

Meanwhile, I have added mypy checking to the build, as adding type hints without enforcing them easily leads to errors. This did cause conflicts with your changes though. Can you please rebase? Thanks.

@akx
Copy link
Contributor Author

akx commented Jun 19, 2024

Can you please rebase? Thanks.

Done, let's see if it passes! 🤞

adding type hints without enforcing them easily leads to errors.

Tangentially: I know we've talked about Ruff before and I know how you feel about it, but it was pretty useful here, as the ANN series of rules can narrow down places that are missing annotations, and a human can then figure them out.

@pennersr
Copy link
Owner

See #3905 -- some of your changes are not Python 3.7 compatible, that will be addressed by #3905.

@akx
Copy link
Contributor Author

akx commented Jun 20, 2024

@pennersr On that note, what's your stance on using from __future__ import annotations and futuristic annotations (e.g. list[Foo] | None)? I don't think anything in allauth is expecting to introspect annotations (where unevaluated futured annotations could be an issue).

@pennersr pennersr merged commit 0766e4a into pennersr:main Jun 20, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants