Add type annotations

[jtojnar]

While trying to wrap my head around the codebase to debug #5, I started adding type hints. Posting it here in case you find it useful.

[mgedmin]

Thank you!

To be complete, this PR needs

• a fix for CI failures (test coverage claims line 121 is not tested, the abstract method needs a pragma: nocover)
• a CHANGES.rst note about the dropped Python 2.7 support

To avoid bitrot of type annotations I would also strongly like to have

• a mypy environment in tox.ini, similar to https://github.com/mgedmin/gitlab-trace/blob/master/tox.ini#L34-L39, except no types-requests of course)
• a mypy job in GitHub actions workflow (one liner a la https://github.com/mgedmin/gitlab-trace/blob/master/.github/workflows/build.yml in the list of non-python-version tox envs)

The comments on the diff about my preferences are not blockers.

[mgedmin]

a CHANGES.rst note about the dropped Python 2.7 support

Please do a git pull before touching that file, because I just released 1.3.0 with a bugfix, so now there's a new header in there.

[jtojnar]

I added a GitHub action however Mypy does not really like the dynamic __getattr__ dispatch:

strace_process_tree.py:489: error: Cannot instantiate abstract class "Theme" with abstract attribute "__getattr__"  [abstract]

I can do one of the following:

• Change it back to non-abstract class (and raise NotImplemented)

• Use # type: ignore annotation when creating Theme in main

• Create real formatting methods explicitly.

• Create real formatting methods using descriptors

  class AnsiTheme(Theme):
      normal = AnsiStyle('\033[m')
      red = AnsiStyle('\033[31m')
      green = AnsiStyle('\033[32m')
      blue = AnsiStyle('\033[34m')

[jtojnar]

I came up with a cleaner descriptor-like API without the use of descriptors.

[mgedmin]

The PR is getting big and perhaps ought to be split up?

I'm not sold on the Theme refactoring.

CI seems to be broken.

[mgedmin]

Clearly I went overboard with the Theme magic. It was an experiment to see how to add configurable color highlighting without affecting too much of the code. I still like passing around Theme objects and invoking theme.thing() before printing things. I regret abusing Theme.__new__ instead of writing a standalone def pick_theme(unicode: Optional[bool], color: Optional[bool]) -> Theme.

I'm undecided about the magic __getattr__. I'd like to study the problem of mypy dealing with __getattr__ in greater detail, which will require me to find a block of time.

[jtojnar]

The first issue is that self.ctlseq is not defined in Theme so Theme.__getattr__() cannot refer to it.

I can move Theme.__getattr__() to AnsiTheme.__getattr__() but then it will not be possible to call it on Theme values.

I can resolve that by having Theme.__new__() return Union[AnsiTheme, PlainTheme instead of Theme which will probably satisfy mypy but the dynamic declaration of style properties still makes the code harder to reason about than necessary. Which is why I propose moving the style properties to class/object instantiation so that they are obvious up front.

[mgedmin]

Would something like

StyleName = str
ColorName = str

class StyleProperty:
    name: StyleName
    def __get__(...): ...
    def __set_name__(...): ...

class Theme:
    pid = StyleProperty()
    process = StyleProperty()
    ....
    reset: str = ''
    colors: Dict[ColorName, str] = {}
    styles: Dict[StyleName, ColorName] = {}

class PlainTheme(Theme):
    pass

class AnsiTheme(Theme):
    reset = ''
    colors = {
        'red': ...
    }
    styles = {
        'pid': 'red',
    }

def pick_theme(...) -> Theme:
    ...

work? With some magic in StyleProperty.get that needs to return a callable (sort of like a custom bound method) that remembers which theme it came from, and the name of the style property, and can look up theme.colors.get(theme.styles.get(style_name), '') and then wrap the argument between it and theme.reset?

[jtojnar]

I do not see a benefit over the current proposal.

Other than pick_theme() being a function rather than class method of ThemeFactory. But then we would need to either define the helpers like should_use_color inside the pick_method function, or move them outside, losing the grouping and encapsulation provided by the class.

[mgedmin]

Ah, CI claims this line is not tested (on 3.8).

I usually do

try:
    from typing import Literal  # Python >= 3.8
except ImportError:
    from typing_extensions import Literal

and mention except ImportError in .coveragerc's exclude_lines.

[jtojnar]

I tried that at first but mypy does not like that: https://mypy.readthedocs.io/en/stable/runtime_troubles.html#using-new-additions-to-the-typing-module

[mgedmin]

Curious: how does mypy complain about the try/except version?

[mgedmin]

FWIW I'm not married to the try/except suggestion. I low-key like it because it requires only one pragma to satisfy coverage.py, as the import statement inside the try block is always executed anyway. The sys.version check would # pragma: nocover comments on both import statements.

[jtojnar]

strace_process_tree.py:37: error: Incompatible import of "Literal" (imported name has type "typing_extensions._SpecialForm", local name has type "<typing special form>")  [assignment]

[mgedmin]

Wow, that's a lot of work. I've never tried to add type annotations to my tests before.

Anyway, mypy complains:

tests.py:5: error: Cannot find implementation or library stub for module named "pytest"  [import]
tests.py:5: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
tests.py:80: error: Untyped decorator makes function "test_parse_timestamp" untyped  [misc]

[mgedmin]

tests.py:80: error: Untyped decorator makes function "test_parse_timestamp" untyped [misc]

I wonder if installing types-pytest would help with this. Assuming types-pytest exists. If not, we may have to abandon the idea of type-checking the test suite, because I'm not willing to give up @pytest.mark.parametrize().

[jtojnar]

Wow, that's a lot of work. I've never tried to add type annotations to my tests before.

Most of that was done with search and replace so it was fine.

Anyway, mypy complains:

It passes for me locally. Maybe tox just needs to install pytest, trying that.

[mgedmin]

Whoops, I forgot that tox -e mypy is a different environment that doesn't have pytest installed.

(I'm suddenly seeing the point of having a 'test' extra that depends on pytest, that I used to find a bit weird when I saw it used by other projects.)

[jtojnar]

I am not sure what ”having a 'test' extra that depends on pytest“ means.

[mgedmin]

It's not important.

(It means having an extra_requires={'test': ['pytest']} in setup.py or whatever the equivalent is in pyproject.toml, and a [testenv] extras = test` in tox.ini.)

[jtojnar]

I have dropped the pyproject.toml commit as we need to pull in types-setuptools anyway.