8000 Issue 1348 followup by neiljp · Pull Request #7222 · zulip/zulip · GitHub
[go: up one dir, main page]
Skip to content

Issue 1348 followup #7222

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

Issue 1348 followup #7222

wants to merge 3 commits into from

Conversation

neiljp
Copy link
Contributor
@neiljp neiljp commented Oct 29, 2017

Following up from PR #7202 looking at resolving #1348, these seem like most of the remaining obvious tidying commits (pending the following comments):

  • Tie the statsd_increment decorator function return type (using ReturnT instead of using Any)
  • FuncT was unused in decorator.py, but was used in profile.py, so one commit adds more explicit return typing to the other decorator
  • The typing of return_success_on_head_request was quite inaccurate and is hopefully better now.

Two commits switched to the python3 type annotations, since I saw that in another commit following #7202, but I'm not sure if that's the consensus when changing a function, anything in a file, or if generally it's going to happen in a separate migration?

Also, using the explicit Callable[..., SomeType] format may work around python/mypy#1927, though is arguably more verbose; should we keep to using an #ignore or use something like this if it works?

One reason for the bad typing sneaking through is that as much as we type with HttpRequest and HttpResponse, they are actually synonyms for Any (based on reveal_type, at least). I know there was some work on this, and there's an external project I've seen for django, though I'm not sure how close to use any of that is.

FYI @gnprice @timabbott

neiljp added 3 commits October 28, 2017 22:28
@neiljp
mypy: Improve constraint on return types in statsd_increment decorator.
eecc282
@neiljp
mypy: Improve typing to profile.py & remove FuncT from decorator.py.
c34e361 
FuncT was unused in decorator.py, and only imported into profile.py.
The @Profiled decorator is now more strongly typed on return-type.
Annotations were converted to python3 format.
@neiljp
mypy: Correct return_success_on_head_request typing.
1b32a42 
Also switch to python3 type annotations.
@timabbott
Copy link
Member

These look good, merged, thanks @neiljp!

@ethanhs was doing some work on using a Django stubs project in https://github.com/zulip/zulip/pull/5309/files. I think that got reasonably close to passing clean before he ran out of time; picking up that effort would be the right next step if we want to fix the HttpRequest/HttpResponse Any problem.

@timabbott timabbott closed this Oct 30, 2017
@timabbott
Copy link
Member

We're doing a general migration to the Python 3 syntax (hoping to use some automated tooling other folks have developed; chat with @rht and @gn 71CB price for details). Generally not changing it when editing a function, but if we're redoing the annotations for a function anyway, it can be worth converting it.

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.
3 participants
@neiljp @timabbott @zulipbot
0