pyinfra icon indicating copy to clipboard operation
pyinfra copied to clipboard

python.raise_exception() docs give wrong signature

Open rsyring opened this issue 11 months ago • 3 comments

Describe the bug

I believe the example from the docs for this function is wrong:

python.raise_exception(
    name="Raise NotImplementedError exception",
    exception=NotImplementedError,
    message="This is not implemented",
)

It produces the error:

    [@local] Unexpected error in Python callback: TypeError('NotImplementedError() takes no keyword arguments',)

It should actually be:

python.raise_exception(
    NotImplementedError,
    "This is not implemented",
    name="Raise NotImplementedError exception",
)

Meta

    System: Linux
      Platform: Linux-6.8.0-51-generic-x86_64-with-glibc2.39
      Release: 6.8.0-51-generic
      Machine: x86_64
    pyinfra: v3.1.1
      click: v8.1.3
      configparser: v7.1.0
      distro: v1.9.0
      gevent: v24.11.1
      jinja2: v3.1.2
      packaging: v24.2
      paramiko: v3.5.0
      pytest: v7.4.0
      pytest: v7.4.0
      python-dateutil: v2.8.2
      pywinrm: v0.5.0
      setuptools: v75.8.0
      typeguard: v4.4.1
      typing-extensions: v4.12.2
      wheel: v0.40.0
    Executable: /home/rsyring/.local/pipx/venvs/kilo/bin/pyinfra
    Python: 3.11.7 (CPython, GCC 13.2.0)

rsyring avatar Jan 09 '25 00:01 rsyring

I can reproduce the issue.

One downside of your proposed fix is the inconsistency with all the other operations which allow the global name argument as the first argument :/

But I don't know how this can be fixed apart from hardcoding which params are passed to the exception constructor...

simonhammes avatar Jan 15 '25 19:01 simonhammes

Thanks for getting back to me on this.

One downside of your proposed fix...

Sorry for the confusion. I wasn't actually proposing the fix. I was just documenting what works:

python.raise_exception(
    NotImplementedError,
    "This is not implemented",
    name="Raise NotImplementedError exception",
)

That's how I had to use it for it to work without an error. I'm not saying that should be how it works in the future. If you need to change the code so the call works like the docs example, that seems just fine to me. It would just be a backwards incompatible change.

rsyring avatar Jan 15 '25 19:01 rsyring

I wasn't actually proposing the fix. I was just documenting what works:

Ah, I'm sorry, I misread the issue description.

If you need to change the code so the call works like the docs example, that seems just fine to me. It would just be a backwards incompatible change.

Hmm. Imo the incorrect docs example should be fixed; the inconsistency could always be fixed later. Mind creating a PR to fix the docs?

@Fizzadar Do you have any ideas how the kwargs handling could be improved?

simonhammes avatar Jan 15 '25 20:01 simonhammes