Conversation
|
Alternatives are:
|
bmw
left a comment
There was a problem hiding this comment.
Thanks for doing this. I think it required a fair bit of work.
This PR also disables the strict optional checks that have been enable by default in recent versions of mypy. Once this PR is merged, I will create an issue to study how these checks can be enabled.
I think we should definitely look into this further in the near future as I personally don't fully understand the tradeoffs here. I think you looking into this further or creating an issue to do so would be great.
As for typing.Protocol, I'm honestly somewhat tempted to just do the easiest thing for us and declare a runtime dependency on typing_extensions. I think we may want to consider this in the future, but for now, since you've already come up with a solution that allows us to get away without requiring it that I think will work well, I think we should stick with that for now.
certbot/certbot/_internal/log.py
Outdated
| super(MemoryHandler, self).close() | ||
| self.target = target | ||
| if target: | ||
| self.target = target |
There was a problem hiding this comment.
nit: Can we make the changes in this file another way? target should always exist and I'd rather not ignore the error if it doesn't.
There was a problem hiding this comment.
I removed the fallback to None in getattr, so an AttributeError will be raised with target does not exist in the current instance.
|
To answer to your general comment @bmw, I have already started the work to fix types when the strict mode is enabled on top of this PR. So I will be able to provide the second part of it very soon after this PR is merged. |
| # group, appropriate permissions for the "Everyone" group, and all permissions to the | ||
| # "Administrators" group + "System" user, as they can do everything anyway. | ||
| def chmod(*unused_args, **unused_kwargs): | ||
| def chmod(*unused_args, **unused_kwargs): # type: ignore |
There was a problem hiding this comment.
I personally think it'd be a good idea to include a comment explaining the use of type: ignore in this file. I think it probably makes the most sense to just do it once higher up in the file.
There was a problem hiding this comment.
To be clear, you want to set a global type: ignore in the file to not do it for each function declaration, and also add an overall comment about it?
There was a problem hiding this comment.
I initially was suggesting to keep the type: ignore lines local but add a global comment about it since I'm assuming mypy is essentially complaining about the same problem so why repeat ourselves, but I think if you want to make it a file level type: ignore that's fine too. I think that scope is a little larger than what we really want/need, but only barely and it could save us cycles adding new type: ignore lines to satisfy mypy in this file in the future.
There was a problem hiding this comment.
In fact, defining type: ignore for the entire module is a way wider scope than what I expected. With it, mypy just does not take account anything from the module, making it raise hundreds of errors of type "certbot.compat.os module does not have method..." everywhere this module is used in the python code.
So I just kepts the local type: ignore directives, and added an overall comment about these directives.
|
For myself as much as anything else, the only other comment I wish was part of my review that GitHub posted prematurely without my permission is #8748 (comment). |
|
Have you looked at my comment here also? #8748 (comment) |
|
Yeah. See #8748 (comment). Sorry for the confusing review. It's my usual problem of reviewing PRs across multiple tabs which GitHub doesn't like. |
Built on top of #8748, this PR reenables mypy strict mode and adds the appropriate corrections to pass the types checks. * Upgrade mypy * First step for acme * Cast for the rescue * Fixing types for certbot * Fix typing for certbot-nginx * Finalize type fixes, configure no optional strict check for mypy in tox * Align requirements * Isort * Pylint * Protocol for python 3.6 * Use Python 3.9 for mypy, make code compatible with Python 3.8< * Pylint and mypy * Pragma no cover * Pythonic NotImplemented constant * More type definitions * Add comments * Simplify typing logic * Use vararg tuple * Relax constraints on mypy * Add more type * Do not silence error if target is not defined * Conditionally import Protocol for type checking only * Clean up imports * Add comments * Align python version linting with mypy and coverage * Just ignore types in an unused module * Add comments * Fix lint * Work in progress * Finish type control * Isort * Fix pylint * Fix imports * Fix cli subparser * Some fixes * Coverage * Remove --no-strict-optional (obviously...) * Update certbot-apache/certbot_apache/_internal/configurator.py Co-authored-by: Brad Warren <bmw@users.noreply.github.com> * Update certbot/certbot/_internal/display/completer.py Co-authored-by: Brad Warren <bmw@users.noreply.github.com> * Cleanup dns_google * Improve lock controls and fix subparser * Use the expected interfaces * Fix code Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
In #8748 (comment) we discussed about changing the dict used to set OS options for Apache configurators into a dedicated object. * Create _OsOptions class to configure the os specific options of the Apache configurators * Fix tests * Clean imports * Fix naming * Fix compatibility tests * Rename a class * Ensure restart_cmd_alt is set for specific OSes. * Add docstring * Fix override * Fix coverage
Fixes #8425
This PR upgrades mypy to the latest version available, 0.812.
Given the advanced type inference capabilities provided by this newer version, this PRs also fixes various type inconsistencies that are now detected. Here are the non obvious changes done to fix types:
Protocolclasses, as recommended by mypy (https://mypy.readthedocs.io/en/latest/more_types.html#mixin-classes, https://mypy.readthedocs.io/en/stable/protocols.html)castwhen we are playing withUniontypesThis PR also disables the strict optional checks that have been enable by default in recent versions of mypy. Once this PR is merged, I will create an issue to study how these checks can be enabled.