Clean up errors in mypy --check-untyped-defs
#2471
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dgw
commented
Jun 12, 2023
There was a problem hiding this comment.
I took the first round of review myself, and wound up flagging some stuff to think about/fix not only in the commits from @SnoopJ I hadn't looked at yet, but in some of my own as well.
Push incoming shortly with tweaks I can immediately address in fixup commits, and I left a couple line notes with stuff to do during the eventual rebase/squash of those fixups.
SnoopJ
force-pushed
the
mypy-untyped-defs
branch
from
50ff13e to
0652bca
Compare
Exirel
requested changes
Jun 15, 2023
SnoopJ
force-pushed
the
mypy-untyped-defs
branch
from
28218c6 to
8209feb
Compare
dgw
added a commit
to sopel-irc/sopel-meetbot
that referenced
this pull request
Jun 16, 2023
Taken from a work-in-progress branch, part of sopel-irc/sopel#2471 Exact commit: 13b93895e56e1e61f72187971e4a5c65acf7c076 (not expected to be merged, so the file and its source information will have to be updated once that PR is finished)
This was referenced Jun 17, 2023
Should be pretty close now. |
dgw
force-pushed
the
mypy-untyped-defs
branch
2 times, most recently
from
123f7ec to
16b6e53
Compare
|
Just marked the last thread (that isn't a reminder for the upcoming rebase/squash) as resolved. Big re-review request for @Exirel now put in. 😁 (I heartily recommend using the "Show changes since your last review" tool for this one, at least to start. You can still re-review the whole patch afterward if you're happy with the individual resolutions.) |
Exirel
approved these changes
Jun 21, 2023
Perhaps the result of two different branches being merged to master in the "wrong" order? That's my best guess, because my last changes to this file passed `mypy sopel` just fine.
`int` has `bit_length()` as a method, which is not part of the interface defined by `numbers.Integral`. Therefore, checking that the arguments passed are merely of type `numbers.Integral` is a type violation. Solution: Check that the arguments to be handled are `int` instead.
Complaining loudly if Sopel can't parse its own version number is
probably a net win, honestly.
I'm less convinced that it matters whether the variable and type names
in `version_type = namedtuple('version_info', ...)` match, but meh:
fixing that particular mismatch is cheap.
mypy does not like assigning a list to an existing variable that was already assigned as a generator. This is fair. It'd likely be equally happy with annotating `items` as an Iterable, but I'm not type-hinting whole files in this housekeeping pass. Likewise, later in the file, mypy does not like assigning a tuple to a variable that was declared initially as an empty list. That's also fair, but it also didn't like replacing the initial `actually_disabled = []` with `actually_disabled = tuple()`, so I simply removed it. There's no risk of the variable being undefined, as the comprehension expression always runs (even if it yields zero items).
mypy gets upset at subscripting a dict using an Optional[str] key, and
rightly so. The following code above the option-parsing line always adds
the 'start' action to the command line if there is no other action set:
elif argv[0].startswith('-') and argv[0] not in global_options:
# No sub-command and no global option
argv = ['start'] + argv
The parser will always have an 'action' to pass in, therefore we don't
need the third argument to set `None` as the `getattr()` default.
Using `self.users.get(self.nick)` tricks mypy into thinking the user object is Optional, leading to a potential missing 'hostmask' attribute. At this point in the function, we know that `self.nick` exists in `self.users` already, so just directly fetch it to avoid the ambiguity.
Since `iri_to_uri()` uses `urlencode_non_ascii()`, which I modified in a somewhat non-intuitive way to make `mypy --check-untyped-defs` happy, I wanted to add at least a few test cases to make sure the behavior didn't change due to my edit. Thankfully, both before and after this patch to `tools/web.py`, the output of `iri_to_uri()` matches what I get from online punycode en/decoder tools, which validates both the patch and the original code. This patch also fixes assigning a generator to what used to be a local list variable in `search_urls()`. tools.web: fix another typing error, in `search_urls()`
Hinting the whole file can wait, but I really want to at least make `mypy --check-untyped-defs sopel` pass, even if it's still not checking 100% of the code due to missing hints in some files.
Once again, needed to add one (1) type hint for mypy to be happy. "Cannot assign to a method" in `ValidatedAttribute` gets type-ignore comments and a TODO because we should rework this code not to do that.
mypy needs to know the type of `example.result`, and it wouldn't have made much sense to add only that without also telling mypy what the function argument being assigned to the instance attribute was... and it wouldn't make sense to type-hint only one argument to the constructor method. So here we are, with type-hints for ALL the constructor parameters, and improved logic for dealing with `result` & `ignore` (both of which take string OR an iterable OF strings). Wanted to type-hint `example.__call__()` too, but that'll require some help from custom types defined elsewhere (that don't yet exist).
Co-authored-by: dgw <dgw@technobabbl.es>
Co-authored-by: dgw <dgw@technobabbl.es>
We've done a bunch of work on typing, and verified that mypy 1.3 doesn't report any new errors. In fact, in `mypy --check-untyped-defs`, a few of the errors go away thanks to bug fixes upstream.
With this, even `mypy --check-untyped-defs` is happy in this file. It might not be perfect, but everything that *can* be checked is correct.
New type-hinted parser attribute to store the parsed data instead, and delete the temporary value used during parsing. Said new type hint isn't doing much work yet since the code that eventually assigns to the new attribute isn't hinted, but I don't want to change too much in this particular branch; we're ONLY worried about actual mypy errors right now. Hinting more stuff for real comes later.
One temporary variable to hold the raw data, and a *second* to hold the processed values, is the way to go in mypy-land.
"Handle" as in, move the common params to a type-hinted module variable and merge a copy of them with the page title in `_update_tld_data()`.
We've fixed (almost) all of these errors. Let's make it easier to avoid introducing new ones.
|
Squash done. Unusually (for me), the force-push diff is not a no-op, because in addition to squashing fixups & dropping commits that were later reverted, I also dropped the last three (converting some annotations to pull from Just worked with @SnoopJ on IRC to split/combine a few more commits and fix broken intermediate states where the existing checks with pytest/flake8 fail. Now running a check through the rest of the branch in case there's another broken intermediate commit that I missed in the first check, before force-pushing one last time. |
|
And, it's done! 49 commits & oopsies 😉 squashed down to 31 logical (for the most part?) steps, with—according to the checks I ran on each commit during the most recent rebase—no test-failing intermediate stages! Both pytest and flake8 passed for every one of the 31 steps it took to get here. 🥳 |
4 tasks
4 tasks
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Made in collaboration with @SnoopJ, this patch resolves about 70 type-checking errors between the two of us. #2462 took care of errors in typed defs, and this PR takes care of the untyped ones.
Along the way, we also confirmed that upgrading to
mypy1.3 works without any new errors (it actually fixed a couple that were due to bugs in oldermypyreleases), and as the final commit this patch also changes the default behavior ofmake mypyto include--check-untyped-defs.Specifically requesting @Exirel's review as our resident typing expert, rather than just any rockstar (sorry y'all!).
Note: There are two unfixed errors in
sopel/modules/remind.pyif only this patch is applied. They're fixed separately in #2470 (which is a bugfix, rather than housekeeping, patch.)Checklist
make qa(runsmake qualityandmake test)