Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

continue on error #516

Closed
ktbarrett opened this issue Dec 21, 2021 · 9 comments
Closed

continue on error #516

ktbarrett opened this issue Dec 21, 2021 · 9 comments

Comments

@ktbarrett
Copy link
Contributor

How would this feature be useful?

I have nox set up to run checks. If a check fails, then nox immediately fails the session and none of the remaining checks are run.

Describe alternatives you've considered

session.run's error_codes arguments is an expected failure. It's not what i'm looking for. I want the session to fail if one of the checks fails, but I want all the checks to run.

You could manually wrap each session.run in a try-catch, collect the exceptions while allowing the test to continue, and re-raise one of the exceptions at the end.

Describe the solution you'd like

I see that session failure occurs when a session.run throws a CommandFailed, this makes things difficult. What may be possible is to not raise that exception when a test fails, but instead signal the session object through some side channel that the run failed. That should allow the session to continue, but fail overall.

@ktbarrett
Copy link
Contributor Author

ktbarrett commented Dec 21, 2021

Perhaps something like the following? try_run would run and squash command failures, but record them in the session object so the session fails at the end. This approach allows the user to mix hard failures with soft failures by using different run APIs.

@nox.session(reuse_venv=True)
def check(session):
    build_env(session)
    session.run("git", "stash", "-k", "-u")
    session.try_run("isort", "--check-only", "--profile=black", ".")
    session.try_run("black", "--check", ".")
    session.try_run("mypy", "--strict", ".")
    session.run("git", "stash", "pop")

EDIT: improved example

@cjolowicz
Copy link
Collaborator

cjolowicz commented Dec 21, 2021

Have you considered running the checks in separate Nox sessions? By default, Nox will then run all checks and report failure if any check fails.

I'd probably leave the git integration to pre-commit, as a mature solution for this kind of thing. As discussed in #515, a common setup is to invoke pre-commit from Nox. You could then use a standard pre-commit configuration with isort and black. As for mypy, I prefer to run this as a dedicated Nox session, because that makes it easy to install my package and dependencies, allowing mypy to import their types. (Plus I can easily typecheck on multiple Python versions.)

@ktbarrett
Copy link
Contributor Author

Have you considered running the checks in separate Nox sessions?

That makes it kind of obnoxious to run them all from the command line. First, you can't use the argument syntax, since it doesn't support multiple sessions, you have to use the environment variable syntax. Second you have to remember all the checks you want to run. Not having to do that is one of the advantages of using nox.

I suppose I could run nox recursively to work-around doing that, but that sounds obnoxious as well.

@cjolowicz
Copy link
Collaborator

cjolowicz commented Dec 22, 2021

Have you considered running the checks in separate Nox sessions?

That makes it kind of obnoxious to run them all from the command line. First, you can't use the argument syntax, since it doesn't support multiple sessions, you have to use the environment variable syntax.

I'm not sure I understand what you mean by "argument syntax" and "environment variable syntax", can you clarify?

FWIW, you can specify multiple sessions like this: nox -s lint mypy.

Second you have to remember all the checks you want to run. Not having to do that is one of the advantages of using nox.

If you want to run all the sessions defined in noxfile.py, you can invoke nox without arguments. If there are sessions that shouldn't run by default, you can define nox.options.sessions in noxfile.py. Like this:

nox.options.sessions = ["lint", "mypy"]

I suppose I could run nox recursively to work-around doing that, but that sounds obnoxious as well.

I don't think that's necessary given the options mentioned above.

@cjolowicz
Copy link
Collaborator

Yet another option would be to have a wrapper session using session.notify.

import nox

def lint(session): ...

def mypy(session): ...

def check(session):
    session.notify("lint")
    session.notify("mypy")

I'd go for the simpler solutions listed above, unless you need added flexibility (e.g. multiple wrapper sessions).

@ktbarrett
Copy link
Contributor Author

ktbarrett commented Dec 23, 2021

FWIW, you can specify multiple sessions like this: nox -s lint mypy.

After looking for it I found it here. I'm not about to scour the documentation, but I don't remember seeing any other examples except the second line in the first example I linked (that I missed) and its not explicitly stated. Could be clearer.

If you want to run all the sessions defined in noxfile.py, you can invoke nox without arguments. If there are sessions that shouldn't run by default, you can define nox.options.sessions in noxfile.py.

Not a good solution. "Include everything, but not this stuff" smells funny. It also means there is only one "set" possible.

Yet another option would be to have a wrapper session using session.notify.

Perfect! Will use this. However, this isn't mentioned in the tutorial and buried in pages of API documentation.

Feel free to close this. I just missed an existing feature that should work for me.

@cjolowicz
Copy link
Collaborator

Closing this, thanks for the feedback!

Do feel free to submit any improvements to the docs. For example, the tutorial mentions --sessions (with an s) here, but an example using the option with more than one argument would indeed be helpful. Also, the docstring for session.notify could get a blurp about how this can be used to implement wrapper sessions.

@gschaffner
Copy link

The session.notify solution doesn't solve this perfectly, as session.notify can only append sessions to the end of the queue. It can't tell Nox to run lint & mypy right after the current session finishes, before the next session in the queue is run. For example: if the noxfile a few comments up is run with nox -s check othersession, othersession will run before lint and mypy.

One idea is to allow notify to have an optional argument to request an append to the left of the queue rather than to the right. notify(..., asap=True)? I have implemented this and it does work, although it has the potentially-confusing behavior that if notifying multiple sessions they must be done in reverse order since it's a left append. (For example, you must call notify("mypy", asap=True) before notify("lint", asap=True) to run lint before mypy.) But the notify API could perhaps be reasonably extended to accept lists as well to make this a bit cleaner:

@nox.session(python=False)
def check(session):
    session.notify(["lint", "mypy"], [lint_posargs, mypy_posargs], asap=True)

I admit, this is still not very elegant.

A minor aside:

asap could also allow for a hacky and bad way of accomplishing #453 (also without the benefits of properly sorting a dependency graph):

@nox.session
def require_1(session): ...

@nox.session
def require_2(session): ...

@nox.session(python=False)
def dependent(session):
    # notify in reverse order since we are appending items to the left of the session queue
    session.notify("dependent_helper", asap=True)
    session.notify("require_2", asap=True)
    session.notify("require_1", asap=True)

@nox.session
def dependent_helper(session):
    # The user shouldn't run this session directly. Unfortunately it will still show in
    #`nox -l`.
    # If run via `dependent`, `require_1` and `require_2` will run before this.
    ...

@gschaffner
Copy link

gschaffner commented Apr 11, 2022

It's also worth noting that if specifying prerequisites gets implemented for #453, this issue would easily be solved by

@nox.session(python=False, requires=["lint", "mypy"])
def check(session): pass

if the requires list was treated as ordered, i.e. where lint is a prerequisite for mypy in the graph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants