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

Implement mypy annotations (Resolves #549) #557

Merged
merged 6 commits into from
Nov 7, 2019
Merged

Conversation

64bitpandas
Copy link
Member

@64bitpandas 64bitpandas commented Nov 4, 2019

This should make improper type usages much easier to catch and encourage better self-documentation in the future :)

Some notes:

  • Typing annotations are very much WIP and I used Any too liberally, especially for request: Any since I was unsure what type request is. request: Any has been fixed and replaced with request: HttpRequest
    Some of the Any keywords are used purposefully, but many can definitely be improved.
  • new_semantic_analyzer hasn't been enabled yet- an internal error occurs (see bottom of comment for stacktrace) and I figured that fixing it would be a whole other unrelated problem. Fixed
  • Perhaps some mypy-related documentation is in order?

Squashed commit: see https://github.com/64bitpandas/ocfweb for full commit history
@emmatyping
Copy link
Member

Ah, yeah we need to update the version of the django plugin to work with the new semantic analyzer. If you could update that and mypy itself, that would be great! I'll start reviewing this a bit later.

@64bitpandas
Copy link
Member Author

Alright sounds good! I've fixed the first 2 notes.

Copy link
Member

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent! Thank you so much for working on it. I have a few nits/suggestions here and there, but otherwise I look forward to this being merged :P

mypy.ini Outdated Show resolved Hide resolved
ocfweb/account/chpass.py Outdated Show resolved Hide resolved
ocfweb/account/register.py Outdated Show resolved Hide resolved
ocfweb/account/vhost_mail.py Outdated Show resolved Hide resolved
@emmatyping
Copy link
Member

Thank you so much for putting in the time on this! It is greatly appreciated.

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