Mypy annotation zerver.lib.event_queue and zerver.lib.queue #958

cpdean · 2016-06-06T00:17:37Z

I ran into some problems with send_event taking either a list of ints or a list of dicts, so i figured i would change the caller's code.

Had problems with events either being a Dict with str keys and one caller who put an int key in there.

Additionally, needed to create some placeholder callbacks where mypy claimed it couldn't infer the type of lambdas.

smarx · 2016-06-06T00:17:38Z

Automated message from Dropbox CLA bot

@cpdean, it looks like you've already signed the Dropbox CLA. Thanks!

timabbott · 2016-06-07T20:31:26Z

Cool, thanks for doing this @cpdean! I merged versions of the first 3 commits (mostly just clarifying commit messages, plus a bit of cleanup to a27e50c70d5d6d675cdaafbd438cf69258ff211d to make it pass mypy checks and remove annotations not directly related to the change).

Can you rebase this branch to squash the fixes to bugs introduced in this branch into the original commit that introduced them? Then it'll be a lot easier to incrementally merge the remaining pieces of this.
check out the Zulip coding style guidelines here for details on our commit style: https://zulip.readthedocs.org/en/latest/code-style.html#version-control

Also, I'm curious about the need for nop_callback -- is that working around a mypy bug? It seems like mypy should be able to handle normal usage of lambda x: None, so it might make sense to report the bug to upstream mypy and get it fixed there...

timabbott · 2016-06-13T16:42:06Z

@cpdean just wanted to check in on this. If you're busy, it'd be valuable to at least have an answer to the nop_callback question...

cpdean · 2016-06-13T17:10:36Z

woops, hey I'll dig into the lambda thing tonight. making commits better
will take me a bit - my git powers are pretty much just limited to squashing

On Mon, Jun 13, 2016, 12:42 Tim Abbott [email protected] wrote:

@cpdean https://github.com/cpdean just wanted to check in on this. If
you're busy, it'd be valuable to at least have an answer to the
nop_callback question...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#958 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAJxYw62xU0gfdV94g58i7pN9vsVppiPks5qLYhngaJpZM4Iue-E
.

cpdean · 2016-06-15T04:49:47Z

@timabbott okay, i've ripped out the nop_callback type things and I'm able to reproduce the error I saw earlier this month.

I boiled it down to a minimal example and reported the error here: python/mypy#1710

Should we have the lambas in place and just add # type: ignore # https://github.com/python/mypy/issues/1710 for now?

cpdean · 2016-06-15T16:51:07Z

woops, my bad. made a duplicate of python/mypy#1425

timabbott · 2016-06-15T19:22:19Z

No worries! The # type: ignore approach linked to python/mypy#1425 sounds good.

cpdean · 2016-06-22T00:31:34Z

added the ignore notes on inline lambdas and cleaned out a third of the commits

it'll be a couple days before i can take another pass but let me know what I should focus on. thanks @timabbott !

Some parts say Dict[text_type, text_type], others store more complicated types than strs. And somewhere someone uses an int as a key in the event dict obj.

waiting on python/mypy#1425

cpdean · 2016-06-22T02:55:27Z

found some more stuff to clean up and make more clear. ready for feedback, thanks again @timabbott !

timabbott · 2016-06-22T18:44:22Z

@sharmaeklavya2 can you do a review pass on this?

sharmaeklavya2 · 2016-06-23T03:57:48Z

zerver/lib/event_queue.py

@@ -54,6 +56,7 @@ class ClientDescriptor(object):
    def __init__(self, user_profile_id, user_profile_email, realm_id, event_queue,
                 event_types, client_type_name, apply_markdown=True,
                 all_public_streams=False, lifespan_secs=0, narrow=[]):
+        # type: (int, str, int, EventQueue, List[str], str, Optional[bool], Optional[bool], Optional[int], Iterable[Sequence[text_type]]) -> None


user_profile_email should be text_type, isn't it?

Are you sure None is a valid value for apply_markdown, all_public_streams and lifespan_secs?

sharmaeklavya2 · 2016-06-24T12:20:42Z

zerver/lib/event_queue.py

@@ -233,13 +254,15 @@ def to_dict(self):

    @classmethod
    def from_dict(cls, d):
+        # type: (Dict[str, Any]) -> EventQueue


d should be Mapping[str, Any].

cpdean · 2016-06-24T12:27:50Z

@sharmaeklavya2 this is great thank you. admittedly I hadn't read the cheat sheet before starting this. I'll be able to clean this up this weekend.

sharmaeklavya2 · 2016-06-24T12:39:52Z

Well I have suggested using Mapping or MutableMapping at some places, but there is a slight chance that might not work. For e.g., if an object has a property that is typed as a Dict, assigning a Mapping to it will fail. I have not checked your annotations very deeply so I might miss out on things like that.

After annotating all this code, you probably know it well enough that you can figure it out yourself what should be the correct type for a variable among Mapping, MutableMapping and Dict. But if you face any problems in doing so, just mention me and I'll try to help.

sharmaeklavya2 · 2016-06-24T13:54:35Z

zerver/lib/event_queue.py


 def receiver_is_idle(user_profile_id, realm_presences):
+    # type: (int, Dict[int, Dict]) -> bool


realm_presences should be Optional[Mapping[int, Mapping]].

sharmaeklavya2 · 2016-06-24T20:09:23Z

@timabbott @cpdean I have reviewed this pull request.

cpdean · 2016-06-26T13:57:28Z

@sharmaeklavya2 i had missed your comment about pika.BasicProperties earlier but have fixed it. Also I fixed the annotation on event_types to set it to List[str] since the other parts of Zulip seem to agree on that type.

sharmaeklavya2 · 2016-06-26T14:16:30Z

zerver/lib/event_queue.py

@@ -56,7 +56,7 @@ class ClientDescriptor(object):
    def __init__(self, user_profile_id, user_profile_email, realm_id, event_queue,
                 event_types, client_type_name, apply_markdown=True,
                 all_public_streams=False, lifespan_secs=0, narrow=[]):
-        # type: (int, text_type, int, EventQueue, List[text_type], text_type, bool, bool, int, Iterable[Sequence[text_type]]) -> None
+        # type: (int, text_type, int, EventQueue, List[str], text_type, bool, bool, int, Iterable[Sequence[text_type]]) -> None


I see that self.event_types is never modified once it is set (not just in this function, but also outside it), so maybe Sequence[str] would be better here.
Actually there are some places where self.event_types is being compared to None, like in ClientDescriptor.accepts_event. It should be Optional[Sequence[str]].

Oh good catch. Fixed it.

sharmaeklavya2 · 2016-06-27T17:42:14Z

zerver/tests/test_subs.py

@@ -781,7 +781,7 @@ def test_successful_subscriptions_add(self):
        are generated.
        """
        self.assertNotEqual(len(self.streams), 0)  # necessary for full test coverage
-        add_streams = [u"Verona2", u"Denmark5"]
+        add_streams = [u"Verona2", u"Denmark5"] # type: List[text_type]


Is this really needed? Isn't mypy able to deduce this? (I haven't checked)

timabbott · 2016-07-04T18:36:12Z

Merged via #1171. Thanks @cpdean for working on this!!

cpdean force-pushed the mypy-annotation-zerver.lib.event_queue branch 2 times, most recently from e73c2eb to abca3fd Compare June 7, 2016 01:58

cpdean force-pushed the mypy-annotation-zerver.lib.event_queue branch from ebe3089 to 21c66f6 Compare June 22, 2016 00:30

cpdean added 7 commits June 21, 2016 21:52

annotate functions that don't return

097672b

first pass annotating queue things

0b92e2e

annotate events, some inconsistencies found through mypy

da68c77

Some parts say Dict[text_type, text_type], others store more complicated types than strs. And somewhere someone uses an int as a key in the event dict obj.

fix pika types

b4c1579

convert str to text_type on narrow param

1c45553

pullout statsd_path cleaning and annotate separately

0e443f6

ignore inline lambda errors

efbcfd1

waiting on python/mypy#1425

cpdean force-pushed the mypy-annotation-zerver.lib.event_queue branch from 21c66f6 to efbcfd1 Compare June 22, 2016 02:54

sharmaeklavya2 reviewed Jun 23, 2016
View reviewed changes

sharmaeklavya2 reviewed Jun 24, 2016
View reviewed changes

cpdean added 3 commits June 25, 2016 12:23

fix misuse of Optional types

0b2e53d

fix ducktyping on iterables and mappings

23bc6d5

fix client name text_type annotation

b61e052

cpdean force-pushed the mypy-annotation-zerver.lib.event_queue branch from 0196eee to b61e052 Compare June 25, 2016 16:56

timabbott force-pushed the master branch from 17a5535 to 28b6f1d Compare June 25, 2016 17:50

cpdean added 2 commits June 26, 2016 09:34

remove import as the fully-qualified type is used in annotations

3a5c5db

change event_types to List[str] as others seem to use that

56f0f15

sharmaeklavya2 reviewed Jun 26, 2016
View reviewed changes

cpdean added 2 commits June 26, 2016 10:20

set event_types to Optional

4baaa28

set current_client_name to an Optional

fe00262

sharmaeklavya2 reviewed Jun 27, 2016
View reviewed changes

timabbott closed this Jul 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mypy annotation zerver.lib.event_queue and zerver.lib.queue #958

Mypy annotation zerver.lib.event_queue and zerver.lib.queue #958

cpdean commented Jun 6, 2016

smarx commented Jun 6, 2016

timabbott commented Jun 7, 2016

timabbott commented Jun 13, 2016

cpdean commented Jun 13, 2016

cpdean commented Jun 15, 2016

cpdean commented Jun 15, 2016

timabbott commented Jun 15, 2016

cpdean commented Jun 22, 2016

cpdean commented Jun 22, 2016

timabbott commented Jun 22, 2016

sharmaeklavya2 Jun 23, 2016

sharmaeklavya2 Jun 23, 2016

sharmaeklavya2 Jun 24, 2016

cpdean commented Jun 24, 2016

sharmaeklavya2 commented Jun 24, 2016 •

edited

Loading

sharmaeklavya2 Jun 24, 2016

sharmaeklavya2 commented Jun 24, 2016

cpdean commented Jun 26, 2016

sharmaeklavya2 Jun 26, 2016 •

edited

Loading

cpdean Jun 26, 2016

sharmaeklavya2 Jun 27, 2016

timabbott commented Jul 4, 2016


		def receiver_is_idle(user_profile_id, realm_presences):
		# type: (int, Dict[int, Dict]) -> bool

Mypy annotation zerver.lib.event_queue and zerver.lib.queue #958

Mypy annotation zerver.lib.event_queue and zerver.lib.queue #958

Conversation

cpdean commented Jun 6, 2016

smarx commented Jun 6, 2016

timabbott commented Jun 7, 2016

timabbott commented Jun 13, 2016

cpdean commented Jun 13, 2016

cpdean commented Jun 15, 2016

cpdean commented Jun 15, 2016

timabbott commented Jun 15, 2016

cpdean commented Jun 22, 2016

cpdean commented Jun 22, 2016

timabbott commented Jun 22, 2016

sharmaeklavya2 Jun 23, 2016

Choose a reason for hiding this comment

sharmaeklavya2 Jun 23, 2016

Choose a reason for hiding this comment

sharmaeklavya2 Jun 24, 2016

Choose a reason for hiding this comment

cpdean commented Jun 24, 2016

sharmaeklavya2 commented Jun 24, 2016 • edited Loading

sharmaeklavya2 Jun 24, 2016

Choose a reason for hiding this comment

sharmaeklavya2 commented Jun 24, 2016

cpdean commented Jun 26, 2016

sharmaeklavya2 Jun 26, 2016 • edited Loading

Choose a reason for hiding this comment

cpdean Jun 26, 2016

Choose a reason for hiding this comment

sharmaeklavya2 Jun 27, 2016

Choose a reason for hiding this comment

timabbott commented Jul 4, 2016

sharmaeklavya2 commented Jun 24, 2016 •

edited

Loading

sharmaeklavya2 Jun 26, 2016 •

edited

Loading