8000 Improve decorator annotation by sharmaeklavya2 · Pull Request #1360 · zulip/zulip · GitHub
[go: up one dir, main page]
Skip to content

Improve decorator annotation #1360

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Improve decorator annotation #1360

wants to merge 3 commits into from

Conversation

sharmaeklavya2
Copy link
Member

I improved the annotations of several decorators. However, it is not possible now to have perfect annotations of parametrized decorators because of python/mypy#1551.

The annotations were able to catch 2 issues, which I have fixed in my first 2 commits.

@sharmaeklavya2
Remove return where no return value is expected.
6537af8 
zerver.lib.notifications.send_future_email has return type None.
So replace `return send_future_email(...` by `send_future_email(...`.
@sharmaeklavya2
zerver/views/streams.py: Annotate variables to help mypy.
053a291
@sharmaeklavya2
Improve annotations of decorators.
9e58977
@timabbott
Copy link
Member

Looks like this fails the linter due to an unused code issue with _T; if you fix that and add links to the relevant mypy issues for all the type: ignores that don't have them, I can merge this.

If you haven't already, you should post comments on the relevant mypy issues noting that they affect Zulip's decorators so they know we're having to use a lot of type: ignores for working around these mypy limitations.

@sharmaeklavya2
Copy link
Member Author

Fixed the _T issue. Also opened python/mypy#1927, which affects almost all decorators whose annotations I have changed.

@timabbott
Copy link
Member

Merged, thanks @sharmaeklavya2!

@timabbott timabbott closed this Jul 22, 2016
@timabbott
Copy link
Member

Maybe also worth posting on python/mypy#1551?

@sharmaeklavya2
Copy link
Member Author

Oh I forgot to add links to ignores. I was about to do that just now but you merged this.

@timabbott
Copy link
Member

Just do it as a new commit; not a big deal

@sharmaeklavya2
Copy link
Member Author

Posted python/mypy#1551 (comment).

@timabbott
Copy link
Member

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sharmaeklavya2 @timabbott
0