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

Check untyped defs in the certbot module #5952

Closed
ohemorange opened this issue May 11, 2018 · 8 comments
Closed

Check untyped defs in the certbot module #5952

ohemorange opened this issue May 11, 2018 · 8 comments

Comments

@ohemorange
Copy link
Contributor

This issue is for checking untyped definitions in the certbot module.

See explanation here: https://docs.google.com/document/d/1fp0tPP9bzYkUViTnOXAyoT_KfgTfs8HSpNMoU8Tm_SY/edit?usp=sharing

From http://blog.zulip.org/2016/10/13/static-types-in-python-oh-mypy/:

Add the --check-untyped-defs option to your mypy arguments, and get that running with no errors on the codebase. This option causes mypy to check every def in the codebase for internal consistency; mypy can detect many classes of bugs and mistakes in your codebase, without your having written a single type annotation!

In most cases, you’ll want to fix bugs and bad code as you go, but you can also use #type: ignore annotations or exclude files to defer issues. For example, we excluded all of Zulip’s tests at first, since they’re both lower value to type-check and had most of the monkey-patching and other questionable Python in the project. For Zulip, getting clean --check-untyped-defs output took me about 2 days of hard work, including merging fixes for about 40 issues in the Zulip codebase.

I spent another day or two generating nice reproducers for the mypy bugs I’d encountered and improving typeshed. Now that mypy is no longer in its infancy, mypy bugs are increasingly rare. But a large project should expect to encounter and fix typeshed issues (just submit a PR!).

Once mypy --check-untyped-defs passes on your codebase, you should lock down your progress by running the mypy checker in your CI environment.

Because mypy type annotations are optional, once you’ve done the setup above, you can start annotating your codebase at whatever pace you like. Over time, you’ll get the benefits of static types in the parts of the codebase that you’ve annotated, without needing to do anything special to the rest of the codebase (the wonders of gradual typing!). In the next section, I’ll discuss strategy for how to move the codebase towards being fully annotated.

@bmw
Copy link
Member

bmw commented May 14, 2018

This is being worked on.

There was a question about how to run tests for a single module or single file. To do this, make sure you've activated the virtual environment as described at the bottom of this section.

To run tests on a single package, run pytest --pyargs <module_name>. For instance, run pytest --pyargs certbot to run all unit tests on the certbot module.

To run tests for a single file/module, find the tests for that module and just run python path/to/file_test.py. The convention for where these test files is located is described at the top of https://certbot.eff.org/docs/contributing.html#testing.

@bmw
Copy link
Member

bmw commented May 15, 2018

FYI, someone is working on this.

@bmw
Copy link
Member

bmw commented May 15, 2018

When doing this, you should change the acme requirement in setup.py in the root of the repo to acme>0.24.0.

@bmw
Copy link
Member

bmw commented May 15, 2018

For the set_by_cli.detector usage, I think we should just use # type: ignore and add a link to python/mypy#2087 for now.

@dmfigol
Copy link
Contributor

dmfigol commented May 15, 2018

No zope support yet:
python/mypy#3960

UPD, WA:

    field = interfaces.IConfig.__getitem__(name)  # type: zope.interface.interface.Attribute
    return field.__doc__

@bmw
Copy link
Member

bmw commented May 15, 2018

Nice digging!

@dmfigol
Copy link
Contributor

dmfigol commented May 16, 2018

certbot/tests/reporter_test.py
we are replacing sys.stdout with six.StringIO()
sys.stdout by default is IO[str]
six.StringIO() is six.StringIO
I haven't found a way to assign the type so that mypy would be happy. Union does not help

@dmfigol
Copy link
Contributor

dmfigol commented May 16, 2018

Submitted PR to fix socket.create_connection type
python/typeshed#2136

dmfigol added a commit to dmfigol/certbot that referenced this issue May 16, 2018
* Fix certbot#5952

* Bump mypy to version 0.600 and fix associated bugs

* Fix pylint bugs after introducing mypy
@bmw bmw added the has pr label May 17, 2018
@bmw bmw closed this as completed in #6005 May 18, 2018
bmw pushed a commit that referenced this issue May 18, 2018
* Prepare certbot module for mypy check untyped defs

* Fix #5952

* Bump mypy to version 0.600 and fix associated bugs

* Fix pylint bugs after introducing mypy

* Implement Brad's suggestions

* Reenabling pylint and adding nginx mypy back
@bmw bmw added this to the 0.25.0 milestone Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants