tutor icon indicating copy to clipboard operation
tutor copied to clipboard

feat: strongly typed hooks

Open regisb opened this issue 3 years ago • 2 comments

Now that the mypy bugs have been resolved, we are able to define more precisely and cleanly the types of Actions and Filters.

Moreover, can now strongly type named actions and hooks (in consts.py). With such a strong typing, we get early alerts of hooks called with incorrect arguments, which is nothing short of awesome :)

This change breaks the hooks API by removing the context=... argument. The reason for that is that we cannot insert arbitrary arguments between P.args, P.kwargs: https://peps.python.org/pep-0612/#the-components-of-a-paramspec

A function declared as def inner(a: A, b: B, *args: P.args, **kwargs: P.kwargs) -> R has type Callable[Concatenate[A, B, P], R]. Placing keyword-only parameters between the *args and **kwargs is forbidden.

Getting the documentation to build in nitpicky mode is quite difficult... We need to add nitpick_ignore to the docs conf.py, otherwise sphinx complains about many missing class references. This, despite upgrading almost all doc requirements (except docutils).

regisb avatar Oct 10 '22 13:10 regisb

FYI @kdmccormick this PR is for solving https://github.com/overhangio/2u-tutor-adoption/issues/121

It was more difficult than I thought to implement this... In particular, getting the docs to compile was challenging. I'd appreciate it if a sphinx/autodoc guru could have a look at my conf.py to try and get rid of the nitpick_ignore entries.

regisb avatar Oct 10 '22 14:10 regisb

Nice. After I put this plugin up for review, I'll take a look at this.

kdmccormick avatar Oct 12 '22 14:10 kdmccormick

Ah, one other thing @regisb , I haven't been able to get Python 3.7 set up on my machine. I'm assuming you were still able to run unit tests under Python 3.7 with these changes.

kdmccormick avatar Nov 10 '22 15:11 kdmccormick

I'm assuming you were still able to run unit tests under Python 3.7 with these changes.

I only just did and realised that positional-only parameters do not work with Python 3.7:

$ make test-lint 
pylint --errors-only --enable=unused-import,unused-argument --ignore=templates --ignore=docs/_ext ./tutor ./tests ./bin ./docs
************* Module tutor.hooks.actions
tutor/hooks/actions.py:113:9: E0001: Parsing failed: 'invalid syntax (<unknown>, line 113)' (syntax-error)
************* Module tutor.hooks.filters
tutor/hooks/filters.py:147:32: E0001: Parsing failed: 'invalid syntax (<unknown>, line 147)' (syntax-error)

Caused by the following pieces of code:

    def add_items(self: "Filter[t.List[E], P]", items: t.List[E]) -> None:
        @self.add()
        def callback(
            values: t.List[E], /, *_args: P.args, **_kwargs: P.kwargs # <------ the slash / is a syntax error in python 3.7
        ) -> t.List[E]:
            return values + items

(and similar in actions.py)

Positional-only arguments were only introduced in python 3.8 by PEP 570: https://peps.python.org/pep-0570/ As far as I know there is no from __future__ import ... statement to backport this feature to python 3.7.

The problem is that if we get rid of this slash then we end up with a mypy error:

$ make test-types 
mypy --exclude=templates --ignore-missing-imports --strict ./tutor ./tests ./bin ./docs
tutor/hooks/filters.py:145: error: Argument 1 has incompatible type "Callable[[Arg(List[E], 'values'), **P], List[E]]"; expected "Callable[[List[E], **P], List[E]]"  [arg-type]
tutor/hooks/filters.py:145: note: This is likely because "callback" has named arguments: "values". Consider marking them positional-only
Found 1 error in 1 file (checked 63 source files)

(Gotta love the irony of the mypy recommendation: "Consider marking them positional-only")

I'll address this issue by adding a # type: ignore. This is unfortunate but I could not find another solution.

regisb avatar Nov 11 '22 11:11 regisb

@regisb Bummer. I fiddled with that line a bit and couldn't find a solution either. Fortunately it seems like the impact is localized to the definition of add_items.

kdmccormick avatar Nov 14 '22 20:11 kdmccormick