Add a 'cwd' keyword argument to the Session.run API #513
Conversation
The 'cwd' keyword argument is intended to optionaly set the current working directory in the sub process and is passed as-is to the underlying subprocess.Popen constructor: https://docs.python.org/3/library/subprocess.html#popen-constructor It is consistent within the existing nox.popen API by thinly and simply wrapping the underlying API.
after command.run both in case of a success or a failure (exit code being 0 or else) We do not check that the cwd stays consistent for the whole time the subprocess is running, hence the behavior of the new cwd keyword argument is undefined *during* the execution of the process. The command.run API feels like it is atomic and not intended to be used in exotic concurent use-cases. This explains why no concurent tests are added at this moment.
|
Just a quick note on the failing checks, apparently it's a known issue with using If we go for this a simple change from cwd: Optional[Union[str, bytes, os.PathLike[str], os.PathLike[bytes]]] = Noneto cwd: Optional[Union[str, bytes, "os.PathLike[str]", "os.PathLike[bytes]"]] = NoneShould resolve the issue 👍🏻 Overall I agree with @theacodes, I think a context manager would feel and look a lot nicer. Consider: @nox.session
def test(session):
with session.chdir("wherever/you/want"):
# Now it's totally clear everything in this block is
# running under "wherever/you/want"As opposed to: @nox.session
def test(session):
# Two different cwd's used here
session.run("pytest", cwd="./somewhere")
session.run("flake8", cwd="./somewhere/else")I would argue the latter is less clear? |
|
Yes, I would also prefer a context manager. I just edited the first post with a "close" command to link with #512. |
|
Thanks for your feedbacks. While I disagree about what would feel and look nicer, since the Also, that kind of context manager would not use anything from the session object, so I'd rather use a context manager that is independent from Farewell! |
I now better understand that sentence. I wasn't aware of the I can't spend much more time on this particular issue at the moment, but again, thanks for the feedback. |
|
We may want to reconsider this if parallel session execution (#544) ever becomes a thing. |
|
See also #555 |
|
Well, in case you @theacodes change your mind about this PR, I'll gladly fix checks, rebase and re-submit it. Edit: sorry about reopening and duplicated comment, I seem to have a hard time commenting on mobile |
Closes #512
Adds a
cwdkeyword argument for running commands. Uses directly the underlyingsubprocess.PopenAPI.