Mypy Compatible type hints for cycler #86

ksunden · 2022-12-08T05:45:43Z

Convert to package instead of single file module
add py.typed (including setup.py changes)
add a CI workflow to run mypy
Add the actual type hints inline

Type hints are generic over both the key type (most commonly strings)
and the value type.

I initially tried to be extra cute and encompass the type expansion that
can happen when cyclers of different types are added/multiplied, but
that proved to be more of a hassle to deal with than I was willing to
deal with initially.

As such, the example from the concat docstring will actually fail to
properly type check with implicit types (as Cycler[str, int] cannot be
concat'd with Cycler[str, str] and properly typecheck)

This may be solvable (resulting in a Cycler[str, int|str]) but that is
not yet implemented.
It may be resolved in downstream code somewhat crudely by typing both
operands explicitly as Cycler[str, int|str] from the outset.

Outside of that corner case, should be entirely correct, I believe.

- Convert to package instead of single file module - add py.typed (including setup.py changes) - add a CI workflow to run mypy - Add the actual type hints inline Type hints are generic over both the key type (most commonly strings) and the value type. I initially tried to be extra cute and encompass the type expansion that can happen when cyclers of different types are added/multiplied, but that proved to be more of a hassle to deal with than I was willing to deal with initially. As such, the example from the `concat` docstring will actually fail to properly type check with implicit types (as Cycler[str, int] cannot be concat'd with Cycler[str, str] and properly typecheck) This may be solvable (resulting in a Cycler[str, int|str]) but that is not yet implemented. It may be resolved in downstream code somewhat crudely by typing both operands explicitly as Cycler[str, int|str] from the outset. Outside of that corner case, should be entirely correct, I believe.

Only lines (almost) it touches outside of what is added for typing are error messages

QuLogic · 2022-12-08T09:06:25Z

You should split out the other changes to separate PRs probably. It's hard to see exactly what's for type hinting here.

ksunden · 2022-12-08T15:57:51Z

Not a whole lot to be pulled out, honestly...

Yeah, I ran a formatter, but 90%+ of what that changed were signature lines that were changed anyway.

The exceptions to this are:

error string text, some of which black formatted to a single line so I got rid of " " between implicitly concatenated strings/changed .format to f strings because flake8 was complaining about line length still.
One or two lines which added space around operators, which flake8 was complaining about anyway.
a few lines of single quote to double quote, which we mostly use double quotes throughout the rest, so that is just getting consistent

I did relax the flake8 CI a little, willing to adjust that, but most of it was needed for this to pass CI.

There are a few small code changes, mostly just covering the theoretically possible types so mypy was happy (many of which have inter-variable relationships that in a valid cycler will never be true cases, but the checks themselves are small and needed for the type checking

ksunden · 2022-12-08T16:01:44Z

cycler/__init__.py

+        self,
+        left: Cycler[K, V] | Iterable[dict[K, V]] | None,
+        right: Cycler[K, V] | Iterable[dict[K, V]] | None = None,
+        op: Any = None,


slightly lazy to use Any here, but this is pretty much zip|np.product|None, exactly but I could not find a good way to type hint that (tried using Literals, but those didn't work with functions and are not available on all supported python versions anyway)

ksunden · 2022-12-08T16:06:51Z

cycler/__init__.py

-        if self._right is not None and old in self._right.keys:
+        if (
+            self._right is not None
+            and isinstance(self._right, Cycler)


explicitly checking isinstance was required for mypy to pass as the option of list[dict[K, V]] which is a seemingly valid type (at least tests blew up when I removed the lines from init which added that type for _right)

That said, if it is valid for _right to be this flattened list of dict instead of a full cycler object, should probably handle the rename like we do for _left

And if it is invalid, should probably figure out why the code is there for it in __init__

Upon more careful inspection, the only test which failed was the one that was called test_starange_init [sic] which is not the recommended way of initializing a cycler, and the cycler object created in that test fails when you try to rename the key of the right child.

So we can:

explicitly disallow right from being an iterable of dicts (while left can be for various reasons)

this would simplify this and some other checks as the only options are cycler or None

change the test to reflect that this is disallowed

explicitly allow this formation, and add tests and correct code where _right is accessed, including at least change_key

add to this test or the change key test a test for this case

Sidenote: this is actually a pretty reasonable example of the kind of problem that mypy can make much more obvious and are not caught, even with "100% of lines coverage" as we have here.

Failing code (adding one line to the strange init test, run on main, this PR would actually make it fall through):

>>> from cycler import cycler, Cycler >>> c = cycler('r', 'rgb') >>> c2 = cycler('lw', range(3)) >>> cy = Cycler(list(c), list(c2), zip) >>> cy.change_key("lw", "ls") ╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮ │ <stdin>:1 in <module> │ │ │ │ /home/kyle/src/scipy/cycler/cycler.py:193 in change_key │ │ │ │ 190 │ │ self._keys.remove(old) │ │ 191 │ │ self._keys.add(new) │ │ 192 │ │ │ │ ❱ 193 │ │ if self._right is not None and old in self._right.keys: │ │ 194 │ │ │ self._right.change_key(old, new) │ │ 195 │ │ │ │ 196 │ │ # self._left should always be non-None │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────╯ AttributeError: 'list' object has no attribute 'keys'

I actually do think this is still unresolved... it passes tests, but the above test case would have the unexpected result of not actually changing the key.

Can right be disallowed from being iterable[dict]? (therefore changing that test)
Or, should the case of right being iterable[dict] be handled as it is for left?

ksunden · 2022-12-08T16:09:18Z

cycler/__init__.py

        # self._left should always be non-None
        # if self._keys is non-empty.
+        elif self._left is None:
+            pass


This explicit check is to type narrow for mypy, it should not ever evaluate to True, could raise a more informative error message instead of just passing, though I suspect even in cases where it would evaluate to true it will blow up earlier.

Could adjust the comment to reflect that the check has been added

ksunden · 2022-12-08T16:13:14Z

cycler/__init__.py

+                for left in self._left:
+                    yield dict(left)
        else:
+            if self._op is None:


mypy was not happy at certain points that op was potentially None... I suspect that since I made op just Any, mypy would not actually complain here anymore, but this and the extra check for the trivial cycler with _left and _right both None were added for mypy reasons

.github/workflows/reviewdog.yml

tacaswell

modulo moving the flake8 config to a config file.

cycler/__init__.py

QuLogic · 2023-04-04T01:03:09Z

cycler/__init__.py

+        if self._right is None or self._left is None:
+            if self._left is not None:
+                for left in self._left:
+                    yield dict(left)


What if self._left is None and self._right is not None? Can that not happen?

I believe that a Cycler with a right child but no left child is invalid, i.e. that all cyclers with only one child (the leaves), it must be the left child.

Whether that is enforced or not, is another question.

Certainly, the recommended constructor (cycler, the function, rather than Cycler, the class) only allows positional args and treats keyword args as label: iterable pairs. As such, that constructor will never yeild a "right only" object

e.g. the _from_iter method directly modifies _left

Note that the previous implementation just directly started iterating on _left

Actually, I realized that most of the code changes in total could be reverted by simply not allowing _left to be None at all. The only way to get the constructor to do it was to do Cycler(None, None). That case was handled in __init__, but even if you do it, it fails to even make a repr, let alone use any of the methods.

Slightly trickier than I thought, as the left=None pathway is used, if only temporarily, in _from_iter, though that is is the only place where _left should be None, and it remedies it right away.

Using empty list ([]) as the sentinel for left instead of None works, though...

…s with different types py39 compat py38 compat flake8

QuLogic

So one other question; if you have Dict[K, V], does that not mean that all values have the same type? I'm not sure if that's supposed to be true, depending on how exact a type you mean.

QuLogic · 2023-04-04T06:01:25Z

cycler/__init__.py

-def _process_keys(left, right):
+def _process_keys(
+    left: Cycler[K, V] | Iterable[dict[K, V]],
+    right: Cycler[K, V] | Iterable[dict[K, V]] | None,


Should more rights be using K, U now to match the other one?

In this case its called only internally and it does kind of need to be consistent here

ksunden · 2023-04-04T17:05:47Z

Yes they need to have the "same" type, but only in the same sense as dict values or lists have the "same" type. It can be a union type, and the built in operators do manage that union expanding (now, that is actually only true as of last night)

QuLogic · 2023-04-05T00:49:49Z

OK, if it can automatically do unions, then that seems okay.

Do you still need to fix #86 (comment) before merging?

ksunden force-pushed the mypy branch from 48c1f5b to 2259efe Compare December 8, 2022 05:47

ksunden added 4 commits December 7, 2022 23:59

blacken

b7c1e16

Only lines (almost) it touches outside of what is added for typing are error messages

Flake8 warnings

b696d81

Limit scope of flake8 workflow to main library, not setup.py

c5263bf

type ignore pytest

40aea83

ksunden commented Dec 8, 2022

View reviewed changes

ksunden changed the title ~~Mypy Compatible tests for cycler~~ Mypy Compatible type hints for cycler Dec 8, 2022

QuLogic reviewed Mar 29, 2023

View reviewed changes

.github/workflows/reviewdog.yml Outdated Show resolved Hide resolved

tacaswell approved these changes Mar 29, 2023

View reviewed changes

Move flake8 config to .flake8

7412cb2

QuLogic reviewed Apr 4, 2023

View reviewed changes

ksunden force-pushed the mypy branch 3 times, most recently from 153b7e0 to 91f32a1 Compare April 4, 2023 05:22

Disallow left of None, properly account for adding/multiplying cycler…

52506c6

…s with different types py39 compat py38 compat flake8

ksunden force-pushed the mypy branch from 91f32a1 to 52506c6 Compare April 4, 2023 05:24

QuLogic reviewed Apr 4, 2023

View reviewed changes

Disallow right child from being iterator of dict

d19e475

QuLogic approved these changes Apr 12, 2023

View reviewed changes

QuLogic merged commit 381d445 into matplotlib:main Apr 12, 2023

Uh oh!

Mypy Compatible type hints for cycler #86

Mypy Compatible type hints for cycler #86

Uh oh!

Conversation

ksunden commented Dec 8, 2022

Uh oh!

QuLogic commented Dec 8, 2022

Uh oh!

ksunden commented Dec 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksunden commented Apr 4, 2023

Uh oh!

QuLogic commented Apr 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants