wntrblm / nox

Flexible test automation for Python
https://nox.thea.codes
Apache License 2.0
1.31k stars 151 forks source link

Unable to pass a function to `session.notify` (misleading docstring?) #748

Open object-Object opened 11 months ago

object-Object commented 11 months ago

Current Behavior

The docs and docstring for session.notify say that the target "may be specified as the appropriate string [...] or using the function object". I assumed this meant you could pass a @nox.session-decorated function into session.notify. However, the type annotation (target: str | SessionRunner) doesn't permit this, and indeed it doesn't work:

nox > Running session notify
nox > Creating virtual environment (virtualenv) using python.exe in .nox\notify
notify
nox > Session notify raised exception ValueError('Session <nox._decorators.Func object at 0x0000028516004310> not found.')
Traceback (most recent call last):
  File "C:\Users\...\venv\Lib\site-packages\nox\sessions.py", line 761, in execute
    self.func(session)
  File "C:\Users\...\venv\Lib\site-packages\nox\_decorators.py", line 75, in __call__
    return self.func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\...\noxfile.py", line 22, in notify
    session.notify(target)
  File "C:\Users\...\venv\Lib\site-packages\nox\sessions.py", line 641, in notify
    self._runner.manifest.notify(target, posargs)
  File "C:\Users\...\venv\Lib\site-packages\nox\manifest.py", line 319, in notify
    raise ValueError(f"Session {session} not found.")
ValueError: Session <nox._decorators.Func object at 0x0000028516004310> not found.

Am I misinterpreting the docstring, or is this not actually supported?

Expected Behavior

Passing a function should run that session as documented.

Steps To Reproduce

With this noxfile:

import nox

@nox.session
def target(session: nox.Session):
    print("target")

@nox.session
def notify(session: nox.Session):
    print("notify")
    session.notify(target)

Run nox -s notify. The above error is raised.

For comparison, this noxfile works as expected:

import nox

@nox.session
def target(session: nox.Session):
    print("target")

@nox.session
def notify(session: nox.Session):
    print("notify")
    session.notify("target")  # this is the only difference

Environment

- OS: Windows 11
- Python: 3.11.0
- Nox: 2023.4.22

Anything else?

It seems like this behaviour/documentation may have been present since all the way back when notify was introduced (d6b3a5ad47d4cc616c821ec1bef82a69ba774fd0)?

Session.notify:

        Args:
            target (Union[str, Callable]): The session to be notified. This
                may be specified as the appropropriate string or using
                the function object.

Manifest.notify:

        Args:
            session (Union[str, ~nox.session.Session]): The session to be
                enqueued.

target appears to be passed verbatim to session, so I'm not sure if this ever actually worked.

cjolowicz commented 8 months ago

The simplest fix would be to compare s.func == session here:

https://github.com/wntrblm/nox/blob/5c82dc553bd04ee017784fc16193da0b35a44ab6/nox/manifest.py#L313

If we go this way, the parameter type would need to change. I don't think there's a reasonable way to get hold of the SessionRunner object in the Noxfile. I'm not sure how we should type the parameter Session.notify and Manifest.notify. Either of these maybe?

https://github.com/wntrblm/nox/blob/5c82dc553bd04ee017784fc16193da0b35a44ab6/nox/registry.py#L26

https://github.com/wntrblm/nox/blob/5c82dc553bd04ee017784fc16193da0b35a44ab6/nox/_decorators.py#L59

They're currently not part of the public interface.

cjolowicz commented 8 months ago

We should also consider changing the docstring and type to simply disallow this. It never worked, and I'm not sure there's a use case for it.

update: On second thoughts, I think there is a case for allowing this. If you pass the function instead of a string, typos can be caught at type-check time (in other words, you get a wiggly line in your editor). It doesn't constrain the order of the function definitions because the notify call happens in the function scope.

henryiii commented 6 months ago

IMO, we should change the docstring for now, and not add this until after working on any reworks planned, like #631 or https://github.com/wntrblm/nox/issues/167#issuecomment-2040967680, as it might complicate those. Long term, I think it makes sense.

MathanGeurtsen commented 1 month ago

If you pass the function instead of a string, typos can be caught at type-check time (in other words, you get a wiggly line in your editor).

This would be great! Here is a workaround I'm currently using to get this:

@nox.session()
def some_session(session: nox.Session):
    session.notify(some_other_session.__name__)

Perhaps this could suggested in the docstring if useful?