mypy: Fix erroring files. #135

alenavolk · 2017-10-20T04:20:38Z

Finally, I have a PR ready :) Things to keep in mind while reviewing:

I annotated strings as Text in files that didn't have any annotations, and as str in files that already had str annotations.
I made a couple of minor changes to the code in places where I thought it made sense. I tested these changes, but it would be great to have someone look over.
I tried to minimize using type: ignore, but you'll still see it in a couple of places. Please let me know if you can suggest a way to avoid that.

Fixes #127.

zulipbot · 2017-10-20T04:20:39Z

Hello @alenavolk, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

alenavolk · 2017-10-20T05:00:24Z

I've just realized that the changes I've made in zulip_bots/generate_manifest.py are causing a problem with /tools/provision. I'll think how to fix that.

timabbott · 2017-10-20T05:16:36Z

@alenavolk it looks like Travis is failing on Python 3.7 and 3.4 because we need to add the typing package to our requirements.txt files.

BTW you should come to our integrations/bots meeting tomorrow at 9AM http://zulip.readthedocs.io/en/latest/chat-zulip-org.html#chat-meetings

roberthoenig

This looks good @alenavolk ! Left some comments. I find it surprising how many lines we have to exclude, all seeminglz because of mypy behaving incorrectly :-/

roberthoenig · 2017-10-20T09:38:43Z

zulip/integrations/log2zulip/log2zulip

@@ -115,7 +115,7 @@ if __name__ == "__main__":
        zulip_client = zulip.init_from_options(args)
        try:
            log_files = json.loads(open(args.control_path, "r").read())
-        except (json.JSONDecodeError, IOError):
+        except (json.JSONDecodeError, IOError):  # type: ignore # error: Cannot determine type of 'IOError'


Hmm, I wonder why this happens.

Have you tried marking this as a Union[] of the two types? I've not needed to annotate exception blocks before, I think. It seems strange it would not determine the Union from the tuple, in any case.

@neiljp In this line any annotation apart from type: ignore results in a misplaced type annotation error.

roberthoenig · 2017-10-20T09:43:42Z

zulip/integrations/perforce/zulip_perforce_config.py

@@ -50,6 +51,7 @@
 # * stream "depot_subdirectory-commits"
 # * subject "change_root"
 def commit_notice_destination(path, changelist):


changelist doesn't seem to be used at all in this functio (but it is passed as an argument in zulip_change-commit.py. @timabbott do you know anything more about this?

roberthoenig · 2017-10-20T09:47:31Z

zulip/tests/__init__.py

@@ -1,2 +1,2 @@
 import pkgutil
-__path__ = pkgutil.extend_path(__path__, __name__)
+__path__ = pkgutil.extend_path(__path__, __name__)  # type: ignore # error: Cannot determine type of '__path__'


Can you do # type: Iterable[Text]? And from typing import Iterable, Text

In typeshed, this is defined as:
def extend_path(path: Iterable[str], name: str) -> Iterable[str]: ...
so I would agree with @derAnfaenger, at this point.

Fixed in both cases, thanks!

roberthoenig · 2017-10-20T09:52:22Z

zulip/tests/test_default_arguments.py

-        with self.assertRaises(SystemExit) as cm, patch('sys.stderr', new=six.StringIO()) as mock_stderr:
+        cm = self.assertRaises(SystemExit)  # type: ignore # error: No variant of "assertRaises" matches argument types
+        mock_stderr = patch('sys.stderr', new=six.StringIO())
+        with cm as cm, mock_stderr as mock_stderr:


Would with cm, mockstderr: suffice here? We already have bound them to variable names before.

Alternatively, if typing is fixed, can the original line just be tidied to something like:
with self.assertRaises(SystemExit) as cm, patch('sys.stderr', new=six.StringIO()) as mock_stderr: ?

For the typing, that assertion appears to be present in the main zulip repo, and presumably type checks ok there, so that might be something to work from or provide a test case for mypy if it comes to that?

@derAnfaenger Not really, it would make the test fail:

ERROR: test_invalid_arguments (tests.test_default_arguments.TestDefaultArguments) ---------------------------------------------------------------------- Traceback (most recent call last): File "/root/python-zulip-api/zulip/tests/test_default_arguments.py", line 27, in test_invalid_arguments self.assertTrue(mock_stderr.getvalue().startswith("""usage: lorem ipsum AttributeError: '_patch' object has no attribute 'getvalue'

@neiljp Could you explain again how this line could be improved if typing is fixed? It looks exactly the same now :)

roberthoenig · 2017-10-20T10:15:13Z

zulip_bots/generate_manifest.py

@@ -68,7 +72,7 @@ def get_docs():
    return doc_paths

 def get_assets():
-    # type: () -> List[str]
+    # type: () -> Iterator[str]


Since map() returns a list, can we do from typing import List and keep the List types? Also for the other occurrences in this file.

That's how it was originally. I changed it to avoid this error: Incompatible return value type (got Iterator[str], expected List[str]).

@neiljp FYI :)

roberthoenig · 2017-10-20T10:27:29Z

zulip_bots/zulip_bots/provision.py

@@ -72,7 +74,7 @@ def main():
    # type: () -> None
    current_dir = os.path.dirname(os.path.abspath(__file__))
    bots_dir = os.path.join(current_dir, "bots")
-    bots_subdirs = map(os.path.abspath, glob.glob(bots_dir + '/*'))
+    bots_subdirs = map(lambda d: os.path.abspath(d), glob.glob(bots_dir + '/*'))


map takes a function as the first argument, so the initial version should be fine here.

It definitely should, but it results in a bunch of weird errors:

Argument 1 to "map" has incompatible type Callable[[AnyStr], AnyStr]; expected Callable[[str], AnyStr] Argument 1 to "isdir" has incompatible type "AnyStr"; expected "Union[bytes, str, _PathLike[Any]]" Argument 1 to "parse_args" has incompatible type Iterator[AnyStr]; expected Iterator[str]

@neiljp FYI :)

roberthoenig · 2017-10-20T10:29:04Z

zulip_bots/zulip_bots/provision.py

@@ -35,6 +36,7 @@ def provision_bot(path_to_bot, force):


 def parse_args(available_bots):
+    # type: (Iterator[AnyStr]) -> argparse.Namespace


What's the reason for using AnyStr instead of str here?

I was just trying to get rid of the errors below. You're right, str works fine here. Fixed.

roberthoenig · 2017-10-20T10:35:38Z

zulip_botserver/tests/__init__.py

@@ -1,2 +1,2 @@
 import pkgutil
-__path__ = pkgutil.extend_path(__path__, __name__)
+__path__ = pkgutil.extend_path(__path__, __name__)  # type: ignore # error: Cannot determine type of '__path__'


Same as for zulip_bots

neiljp

Thanks for this :) I always enjoy seeing more type annotations! There are small things to be changed, some in the PR and some outside in typeshed (and maybe mypy). I look forward to seeing this progress :)

neiljp · 2017-10-20T16:36:47Z

zulip/integrations/irc/irc_mirror_backend.py

@@ -1,6 +1,7 @@
 import irc.bot
 import irc.strings
 from irc.client import ip_numstr_to_quad, ip_quad_to_numstr, Event, ServerConnection
+from typing import Any, Dict


This line makes the file pass mypy, I assume, but is it possible to use a specific type instead of Any in __init__ ?

That's a good point, but I don't think it's possible in this case.

neiljp · 2017-10-20T16:52:08Z

zulip/integrations/log2zulip/log2zulip

@@ -115,7 +115,7 @@ if __name__ == "__main__":
        zulip_client = zulip.init_from_options(args)
        try:
            log_files = json.loads(open(args.control_path, "r").read())
-        except (json.JSONDecodeError, IOError):
+        except (json.JSONDecodeError, IOError):  # type: ignore # error: Cannot determine type of 'IOError'


Have you tried marking this as a Union[] of the two types? I've not needed to annotate exception blocks before, I think. It seems strange it would not determine the Union from the tuple, in any case.

neiljp · 2017-10-20T17:03:39Z

zulip/integrations/zephyr/process_ccache

@@ -10,7 +10,7 @@ ccache_data_encoded = sys.argv[3]

 # Update the Kerberos ticket cache file
 program_name = "zmirror-%s" % (short_user,)
-with open("/home/zulip/ccache/%s" % (program_name,), "w") as f:
+with open("/home/zulip/ccache/%s" % (program_name,), "wb") as f:


Did mypy flag this due to the write on the following line?

Yep, the error is Argument 1 to "write" of "IO" has incompatible type "bytes"; expected "str".

neiljp · 2017-10-20T17:12:43Z

zulip/tests/__init__.py

@@ -1,2 +1,2 @@
 import pkgutil
-__path__ = pkgutil.extend_path(__path__, __name__)
+__path__ = pkgutil.extend_path(__path__, __name__)  # type: ignore # error: Cannot determine type of '__path__'


In typeshed, this is defined as:
def extend_path(path: Iterable[str], name: str) -> Iterable[str]: ...
so I would agree with @derAnfaenger, at this point.

neiljp · 2017-10-20T17:22:10Z

zulip/tests/test_default_arguments.py

-        with self.assertRaises(SystemExit) as cm, patch('sys.stderr', new=six.StringIO()) as mock_stderr:
+        cm = self.assertRaises(SystemExit)  # type: ignore # error: No variant of "assertRaises" matches argument types
+        mock_stderr = patch('sys.stderr', new=six.StringIO())
+        with cm as cm, mock_stderr as mock_stderr:


Alternatively, if typing is fixed, can the original line just be tidied to something like:
with self.assertRaises(SystemExit) as cm, patch('sys.stderr', new=six.StringIO()) as mock_stderr: ?

For the typing, that assertion appears to be present in the main zulip repo, and presumably type checks ok there, so that might be something to work from or provide a test case for mypy if it comes to that?

neiljp · 2017-10-20T17:58:31Z

zulip_bots/setup.py

        try:
-            module = import_module(module_name)
+            module = import_module(module_name)  # type: Any


types.ModuleType ?

It looks very logical here, but for some reason it gives me the Module has no attribute "__version__" error two lines below.

neiljp · 2017-10-20T18:09:04Z

zulip_bots/zulip_bots/test_lib.py

@@ -35,7 +35,7 @@ def get_bot_message_handler(self):
        # handler class. Eventually, we want bot's handler classes to
        # inherit from a common prototype specifying the handle_message
        # function.
-        lib_module = import_module('zulip_bots.bots.{bot}.{bot}'.format(bot=self.bot_name))
+        lib_module = import_module('zulip_bots.bots.{bot}.{bot}'.format(bot=self.bot_name))  # type: Any


types.ModuleType again here?

This one gives me the Module has no attribute "handler_class" error.

neiljp · 2017-10-20T18:11:31Z

zulip_bots/zulip_bots/test_lib.py

@@ -61,7 +61,7 @@ def check_expected_responses(self, expectations, expected_method='send_reply',
        # To test send_message, Any would be a Dict type,
        # to test send_reply, Any would be a str type.
        if isinstance(expectations, dict):
-            expected = [(k, v) for k, v in expectations.items()]
+            expected = [(k, v) for k, v in expectations.items()]  # type: Sequence[Tuple[str, Any]]


This is a list comprehension, in the implementation, so just type it as that?

This line was deleted in master.

neiljp · 2017-10-20T18:15:52Z

zulip_bots/zulip_bots/zulip_bot_output.py

@@ -75,7 +77,7 @@ def main():
        sys.exit(1)

    with patch('zulip.Client') as mock_client:
-        mock_bot_handler = ExternalBotHandler(mock_client, bot_dir)
+        mock_bot_handler = ExternalBotHandler(mock_client, bot_dir)  # type: Any


Is this necessary since this object gets 'monkey patched' later?

I think so, otherwise I get several Cannot assign to a method errors right below (and many others later).

neiljp · 2017-10-20T18:16:31Z

zulip_botserver/setup.py

        try:
-            module = import_module(module_name)
+            module = import_module(module_name)  # type: Any


types.ModuleType ?

Same error as in zulip_bots/setup.py: Module has no attribute "__version__".

alenavolk · 2017-10-20T21:42:26Z

Regarding the failing Travis build. It looks like adding from typing ... imports to these files:

zulip_bots/setup.py
zulip_bots/generate_manifest.py
zulip_botserver/setup.py

causes the tools/provision script to fail with the No module named typing error.

Travis uses this script to recreate the environment every time it runs the tests, that's why the build fails even though all tests have passed locally.

I've tried adding the typing package to requirements.txt, but it doesn't really solve the problem, because these requirements:

-e ./zulip
-e ./zulip_bots
-e ./zulip_botserver

are installed from the same place, and pip looks at their setup.py files before the installation.
(Besides, typing is installed with the ./zulip package later.)

The only solution I can think of at the moment is to change the problematic imports as follows:

if False:
    from typing import Any, Dict, Optional

It fixes the problem locally, and I think it'll make the Travis build pass as well.

timabbott · 2017-10-20T21:44:25Z

@alenavolk I think that means we need to add typing in the setup.py files.

alenavolk · 2017-10-23T20:48:53Z

@timabbott I've added typing as a requirement to both files. For example, in zulip_bots/setup.py:

setuptools_info = dict(
    install_requires=[
        'zulip',
        'mock>=2.0.0',
        'html2text',  # for bots/define
        'typing>=3.5.2.2',
    ],
)

The same thing in zulip_botserver/setup.py. Is that what you had in mind?

Unfortunately, it looks like I'll still have to use the if False trick. Otherwise, the provision script fails (it fails before installing the requirements).

alenavolk · 2017-10-23T21:46:06Z

zulip_bots/zulip_bots/run.py


 from zulip_bots.lib import run_message_handler_for_bot
 from zulip_bots.provision import provision_bot

 current_dir = os.path.dirname(os.path.abspath(__file__))

 def import_module_from_source(path, name=None):
+    # type: (Text, Optional[Text]) -> Any


@neiljp I think this is the only place where I can use types.ModuleType right now.

alenavolk · 2017-10-23T23:06:03Z

@neiljp Thank you so much for reviewing this! I really appreciate it.

Today I tried annotating the module instances with types.ModuleType instead of Any, but I got the following errors:

zulip_bots/setup.py:73: error: Module has no attribute "__version__"
zulip_bots/zulip_bots/test_lib.py:39: error: Module has no attribute "handler_class"
zulip_botserver/setup.py:61: error: Module has no attribute "__version__"

Does it mean I should keep the type: Any annotations in these files?

Also, I know I could use types.ModuleType in annotating zulip_bots/zulip_bots/run.py:

def import_module_from_source(path, name=None):
    # type: (Text, Optional[Text]) -> ModuleType
    ...

but I'm not sure if it makes sense. Wouldn't it be better to keep Any for consistency?

Looking forward to your reply!

timabbott · 2017-10-23T23:20:35Z

@alenavolk if you have a subset of these commits that are fully reviewed and pass tests, it could be good to open those in a new PR so that we can start shrinking this.

Though .. I guess we need to figure out the Travis issue with typing first. @eeshangarg @derAnfaenger do either of you understand the problem?

timabbott · 2017-10-23T23:21:22Z

Actually, looking at the error, I think it's just that generate_manifest.py, which is called from setup.py, is special, and does need the if False hack. We should only need that in a couple files though.

alenavolk · 2017-10-24T21:44:24Z

@timabbott Unfortunately, the error Travis is showing right now is not the only one, there are actually three files failing the Travis build. It's not a complicated problem or anything, but since we don't have a common understanding of how to fix it, I think the best approach would be to first shrink this PR as much as possible and then continue this discussion. I shouldn't have made a PR this big in the first place, but oh well... I'll work with what I've got now :)

timabbott · 2017-10-24T21:56:35Z

@alenavolk which 3 files are failing? I think we might want to just do the if False thing for them...

alenavolk · 2017-10-24T23:09:34Z

@timabbott zulip_bots/setup.py, zulip_bots/generate_manifest.py, and zulip_botserver/setup.py. zulip/setup.py already has the if False hack, otherwise it would be on this list too.

The problem in a nutshell is that typing is included in the standard library only starting from Python 3.5. On the earlier versions we should either install it before installing our packages (zulip, zulip_bots, and zulip_botserver) the same way we install a modern version of pip:

subprocess.call([pip_path, 'install', 'pip>=9.0', 'typing>=3.5.2.2'])
if subprocess.call([pip_path, 'install', '--prefix', venv_dir, '-r',
                    os.path.join(base_dir, requirements_filename)]):
    raise OSError("The command `pip install -r {}` failed. Dependencies not installed!"
                  .format(os.path.join(base_dir, requirements_filename)))

or use the if False hack in our setup files.

We could also just leave these files on the exclude list for the time being. I honestly don't know which of these solutions is cleaner.

zulipbot · 2017-10-27T04:44:31Z

Hello @alenavolk, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

zulipbot · 2017-10-27T04:56:26Z

Hello @alenavolk, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

timabbott · 2017-10-27T06:04:29Z

Merged, thanks @alenavolk! This cleaned up a lot of issues. I think we can probably deal with any remaining type: ignore items in follow-up commits.

zulipbot added the needs review label Oct 20, 2017

roberthoenig reviewed Oct 20, 2017

View reviewed changes

zulipbot added reviewed and removed needs review labels Oct 20, 2017

neiljp reviewed Oct 20, 2017

View reviewed changes

showell force-pushed the master branch from 105db19 to 08e2121 Compare October 21, 2017 00:34

alenavolk commented Oct 23, 2017

View reviewed changes

alenavolk added 2 commits October 26, 2017 17:02

mypy: zulip: Annotate integrations/git/zulip_git_config.py.

af957c1

mypy: zulip: Fix errors in integrations/irc/irc_mirror_backend.py.

995dc33

alenavolk force-pushed the issue-127-mypy branch from 372bee6 to 4b40ce9 Compare October 27, 2017 04:44

zulipbot added needs review and removed reviewed labels Oct 27, 2017

alenavolk added 5 commits October 27, 2017 00:55

mypy: zulip: Fix errors in integrations/log2zulip/log2zulip.

7ad3324

mypy: zulip: Annotate integrations/perforce/zulip_perforce_config.py.

ab192fe

mypy: zulip: Annotate integrations/svn/zulip_svn_config.py.

8becce7

mypy: zulip: Fix errors in integrations/zephyr/process_ccache.

671a078

mypy: zulip: Fix errors in tests/__init__.py.

99785a1

alenavolk added 12 commits October 27, 2017 00:56

mypy: zulip: Fix errors in tests/test_default_arguments.py.

fe6deb2

mypy: zulip_bots: Fix errors in generate_manifest.py.

3d282a1

mypy: zulip_bots: Fix errors in setup.py.

ad050fc

mypy: zulip_bots: Fix errors in zulip_bots/lib.py.

3a20dee

mypy: zulip_bots: Fix errors in zulip_bots/provision.py.

47d8ab8

mypy: zulip_bots: Annotate zulip_bots/run.py.

6546917

mypy: zulip_bots: Fix errors in zulip_bots/test_lib.py.

afb9886

mypy: zulip_bots: Annotate zulip_bots/test_run.py.

5e7f4c5

mypy: zulip_bots: Fix errors in zulip_bots/zulip_bot_output.py.

fd51925

mypy: zulip_botserver: Fix errors in tests/__init__.py.

81205ea

mypy: zulip_botserver: Fix errors in zulip_botserver/server.py.

ed51cab

mypy: zulip_botserver: Fix errors in setup.py.

25d6c68

alenavolk force-pushed the issue-127-mypy branch from 4b40ce9 to 25d6c68 Compare October 27, 2017 04:56

timabbott merged commit 25d6c68 into zulip:master Oct 27, 2017

zulipbot removed the needs review label Oct 27, 2017

alenavolk deleted the issue-127-mypy branch October 27, 2017 16:27

		@@ -35,6 +36,7 @@ def provision_bot(path_to_bot, force):


		def parse_args(available_bots):
		# type: (Iterator[AnyStr]) -> argparse.Namespace

mypy: Fix erroring files. #135

mypy: Fix erroring files. #135

Conversation

alenavolk commented Oct 20, 2017

zulipbot commented Oct 20, 2017

alenavolk commented Oct 20, 2017 • edited Loading

timabbott commented Oct 20, 2017

roberthoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenavolk Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenavolk Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenavolk Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenavolk commented Oct 20, 2017

timabbott commented Oct 20, 2017

alenavolk commented Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

alenavolk commented Oct 23, 2017

timabbott commented Oct 23, 2017

timabbott commented Oct 23, 2017

alenavolk commented Oct 24, 2017

timabbott commented Oct 24, 2017

alenavolk commented Oct 24, 2017

zulipbot commented Oct 27, 2017

zulipbot commented Oct 27, 2017

timabbott commented Oct 27, 2017

alenavolk commented Oct 20, 2017 •

edited

Loading

alenavolk Oct 26, 2017 •

edited

Loading

alenavolk Oct 26, 2017 •

edited

Loading

alenavolk Oct 26, 2017 •

edited

Loading

alenavolk commented Oct 23, 2017 •

edited

Loading