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

Session.posargs should be immutable #400

Closed
rmorshea opened this issue Mar 10, 2021 · 4 comments · Fixed by #439
Closed

Session.posargs should be immutable #400

rmorshea opened this issue Mar 10, 2021 · 4 comments · Fixed by #439

Comments

@rmorshea
Copy link
Contributor

There's no reason that Session.posargs should be mutated by users.

...a tuple would be preferable ...[and] might not even constitute a breaking change

Originally posted by @cjolowicz in #397 (comment)

@cjolowicz
Copy link
Collaborator

Thanks for creating the issue @rmorshea !

Here's an example to illustrate the problem:

import nox

@nox.session
def test(session):
    session.posargs.extend(["-x"])
    session.install("pytest")
    session.run("pytest", *session.posargs)

@nox.session
def lint(session):
    args = [*session.posargs, "src"]
    session.install("flake8")
    session.run("flake8", *args)

The lint session will fail with flake8: error: unrecognized arguments: -x.

@cjolowicz
Copy link
Collaborator

cjolowicz commented Mar 11, 2021

In the context of #397, we discussed two approaches to solve this:

  • Returning a copy using something like global_config.posargs[:]
  • Returning a tuple instead of a list

On further thought, I'm worried that option 2 would break users that expect session.posargs to be a list:

@nox.session
def test(session):
    args = session.posargs + ["--full-trace", "--verbose"]
    # TypeError: can only concatenate tuple (not "list") to tuple

It's true that typically session.posargs is unpacked into session.run, and the precise type of session.posargs is not documented. But by Hyrum's Law some people will depend on it being a list nonetheless.

Option 1 will require some updates to the test suite, but should not break existing Noxfiles. See #397 (comment) for a patch that updates the tests accordingly.

@rmorshea
Copy link
Contributor Author

rmorshea commented Mar 12, 2021

@cjolowicz good point about the list additions. I think I do that myself actually. Copying the list might be the right approach here. I feel like there's a lot more people who will be harmed by changing this to a tuple than would be protected by its immutability.

@vaneseltine
Copy link
Contributor

Yes, flipping through a few examples most of them use generic sequence methods like in and unpacking, but it didn't take long to come across list concatenation in the wild

And I think I might have found a superspreader for one common pattern...

pip:

@nox.session
def lint(session):
    # type: (nox.Session) -> None
    session.install("pre-commit")

    if session.posargs:
        args = session.posargs + ["--all-files"]
    else:
        args = ["--all-files", "--show-diff-on-failure"]

    session.run("pre-commit", "run", *args)

So while a tuple might have been best from the outset, given what's in the wild, copying the list seems like a reasonable approach.

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

Successfully merging a pull request may close this issue.

4 participants