pylint error on decorated `name` argument

[themanifold]

pylint error on decorated name argument Due to the way the name argument is injected into some functions with a decorator, such as pyinfra.operations.server.shell, we get a pylint error like:

deploy/deploy.py:3:0: E1123: Unexpected keyword argument 'name' in function call (unexpected-keyword-arg)

To Reproduce deploy.py:

from pyinfra.operations import server

server.shell(
    name='Run command',
    commands=['echo "1234"']
)

pylint deploy.py

Expected behavior No pylint errors

Meta

• System: Linux Platform: Linux-5.8.11-1-MANJARO-x86_64-with-glibc2.2.5 Release: 5.8.11-1-MANJARO Machine: x86_64 pyinfra: v1.2.1 Executable: /home/user/workspace/common/venv/bin/pyinfra Python: 3.8.5 (CPython, GCC 10.2.0)
• pip installed

[themanifold]

Ah, turns out the support for decorators in pylint is a bit lacking. If you read https://github.com/PyCQA/pylint/issues/259 you'll see that there's a work around. I will test this now.

[themanifold]

OK, so following that information, you do something like this in your .pylintrc:

[TYPECHECK]
signature-mutators=pyinfra.api.operation.operation

See this: https://github.com/PyCQA/pylint/blob/master/doc/whatsnew/2.4.rst

What unfortunately happens is that you will get false negatives when you actually pass in incorrect arguments into the pyinfra decorated functions.

Is flake8 fine with decorators?

I'll leave this up to you as to what you do with it.

[Fizzadar]

So flake8 doesn't do type checking which has prevented this being an issue before! There's currently a PR/branch (https://github.com/Fizzadar/pyinfra/pull/472) tracking implementation of type checking using mypy.

The operation functions are particularly annoying as they all accept a list of global arguments, defined here which is a bit of a nightmare for typing.

I think long run the right approach is to move these into the operation function definition itself, will need to deduplicate that with the deploy function also (possibly by way of a decorator decorator!).

[wookayin]

Not only mypy but also other static type checkers like pyright (as LSP server) will complain about the signature because they cannot process the decorator during static type checking.

One thing worth considering is ParamSpec (PEP 612): see https://github.com/microsoft/pyright/issues/774 and https://peps.python.org/pep-0612/ , but this does not allow introducing additional kwargs to a ParamSpec.

One easy solution to this might be to add dummy **kwargs parameter to all the raw function for operations, e.g.,

 @operation(is_idempotent=False)            
-def shell(commands, state=None, host=None):
-  ...
 @operation(is_idempotent=False)            
+def shell(commands, state=None, host=None, **kwargs):
+  ...

Have you considered this?

[wookayin]

I wish this were included in v2.0 release, but still looking forward to it.

[Fizzadar]

Hm. I can't replicate this (the error) with either mypy or pylint; using the following example:

from typing import Generator, List

from pyinfra.api.operation import operation

@operation
def shell(commands: List[str]) -> Generator[str, None, None]:
    for command in commands:
        yield command

shell(
    name="Run command",
    commands=["echo hi"],
    yes=False,
)

This passes with both tools as the decorator seems to effectively disable all type checking on the function args as it interprets them as *args, **kwargs. So this is useless but also should not be raising the original error here?

---

Regarding fixing typing properly in the future I had one idea - we could (ab)use stub files to force type checkers into including all the global arguments as valid types. Ie by generating (possibly as part of package install?) stub files alongside operation files that type annotate the op functions un-decorated; ie:

def shell(
    commands: list[str],
    name: str | None,
    _sudo: boolean = False,
    ...all the other global args,
) -> Generator[Command | str, None, None]: ...

This would a) get around the Python 3.10 requirement for ParamSpec and b) get around ParamSpec's inability to extend keyword arguments (currently).

Annotations could simply be inlined everywhere in the codebase, including operations, and the stub generator would use these combined with the global arguments to generate stubs for operation modules.

[wookayin]

In my environments: python 3.9.7, pylint 2.13.5. It does give the error as follows (and so does pyright / VS Code, etc.). Looking at upstream issues, I don't think those checkers disable type checking on decorated functions yet as of now.

❯❯❯ pylint gh_476.py
************* Module gh_476
gh_476.py:1:0: C0114: Missing module docstring (missing-module-docstring)
gh_476.py:7:0: C0116: Missing function or method docstring (missing-function-docstring)
gh_476.py:12:0: E1123: Unexpected keyword argument 'name' in function call (unexpected-keyword-arg)
gh_476.py:12:0: E1123: Unexpected keyword argument 'yes' in function call (unexpected-keyword-arg)
...

ParamSpec is available via typing-extensions backport so older python < 3.10 versions can still make use of the new feature. We can use if types.TYPE_CHECKING: blocks to make it completely optional and not affect any of the runtime logics.

But I like the idea of using type stubs as well. pylint will still complain about unexpected-keyword-arg but pylint is not a big deal.

[Fizzadar]

This should now be resolved, first by #817 and now by the fantastic #891.