run app appargs fix for -- double-dash

[itsayellow]

Fixes #267

This PR allows all arguments after pip run <app_name> to be sent verbatim to the <app_name> app. Currently a -- argument is possible to be removed by argparse before sending the list of arguments to the app.

The code included in a new parse_args() function doubles a -- argument if it will be removed by argparse. argparse will only remove the first -- in a line, so this ensures that the appargs specified by the user will all be sent to the app.

[cs01]

A force push was recently done, so this PR will have to be rebased onto origin/master. Apologies for the inconvenience. See #270.

[cs01]

I attempted to rebase this in the appargs_fix branch to save you the effort.

[itsayellow]

A force push was recently done, so this PR will have to be rebased onto origin/master. Apologies for the inconvenience. See #270.

Done--rebased.

[cs01]

This is awesome. pytest has a parameterize decorator that adds some nice reporting if something fails. Recommend using it, but not a big deal. It does make pairing the input with the expected value a little easier to read. The way it's currently written you have to search to match up the elements.

https://docs.pytest.org/en/latest/parametrize.html

[clbarnes]

Agreed, perfect case for a parametrized test. It'll allow each one to pass or fail individually.

Alternatively, zip would be my preferred way to keep input and output iterating in sync with each other.

[cs01]

An intuitive interface to pipx, IMO, with be to support the -- argument like nox does. pipx would forward anything after the first -- to the app. So pipx pycowsay -- hello would say "hello", not "-- hello".

pipx run nox -- -s lint would run nox -s lint

and pipx run nox -- -s lint -- nox_posargs would run nox -s lint -- nox_posargs. It looks like currently the way it's implemented in this PR, it would pass nox -- -s lint -- nox_posargs.

[itsayellow]

I think that's how it works now. But I am confused, because before I was trying to use a pipx run with nox in the past, and it seemed like pipx was removing the -- wherever it was. But now while writing the tests, it appears that pipx only removes the first -- if it is not inside the appargs. i.e. it may only remove the first argument after the app name if it is --.

So somehow it seems to behave differently now than when I was testing it a week or so ago. I don't know if that's my mistake or code difference.

The other thing is that to my mind things get confusing if pipx or its argparse is doctoring anything after pipx run <appname> In my mind I really expect that all to go verbatim to the app, as if the command line was <appname> [...] for pipx run <appname [...].

If we told the user to always put a -- before <appname> then it would basically work fine without any code modification. That was one way I was thinking about addressing this, with just specifying the help text.

[itsayellow]

Now that I see the only unwanted possible -- that is removed after the app name in pipx run is only the first argument after the app name, perhaps I can withdraw the code in this pull request.

@cs01, how about I just add some text to the help to make sure that users know the first -- after the app name needs to be doubled if it is to be sent to the app? And just forget about all the code changes?

[cs01]

@cs01, how about I just add some text to the help to make sure that users know the first -- after the app name needs to be doubled if it is to be sent to the app? And just forget about all the code changes?

I think documenting that -- can be an explicit delimiter between pipx arguments and the app's arguments would be great. I actually didn't know pipx worked like that by default, but I'm glad it does (it's just argparse's default behavior and we get it for free). By documenting that, and maybe a sample nox command on how to pass positional arguments to the app would be good.

[itsayellow]

An intuitive interface to pipx, IMO, with be to support the -- argument like nox does. pipx would forward anything after the first -- to the app. So pipx pycowsay -- hello would say "hello", not "-- hello".

pipx run nox -- -s lint would run nox -s lint

and pipx run nox -- -s lint -- nox_posargs would run nox -s lint -- nox_posargs. It looks like currently the way it's implemented in this PR, it would pass nox -- -s lint -- nox_posargs.

I think the way you prefer seems confusing to me 🙃, especially when you have two apps (like pipx and nox) getting important information (and possibly removing) the --.

My preference would be that everything starting with the app/package name would be passed on verbatim as if instead of pipx run <app> <appargs> you had installed the app on your system and were just typing <app> <appargs> on the command line. I find it confusing if pipx is possibly removing a -- in what would be <appargs> when I wanted to send that -- on to the <app>.

The way out of this currently would be to be explicit, and put the double-dash -- before the app/package name:

pipx run -- nox -s lint -- nox_posargs

That way I can think in my head what I'm really trying to run is

nox -s lint -- nox_posargs

and I don't have to worry that pipx is going to remove a -- somewhere after the nox

Is my example above ok to put as guidance in the help? To put the -- before the app name?

Otherwise to my mind we say, "pipx will remove the first double-dash after the app name, unless you also put a double-dash anywhere beforehand in the pipx command. Also if you put the double-dash later after the app name (not the first argument after the app name), it will be left intact and sent to the app." That is VERY confusing to me.

[clbarnes]

How do either of those options compare to tools like node's npm and ruby's bundler? They're doing more or less the same thing and are extremely widely used in their respective ecosystems. If "least surprise" is a design goal, making pipx consistent with those would be my choice.

[itsayellow]

How do either of those options compare to tools like node's npm and ruby's bundler? They're doing more or less the same thing and are extremely widely used in their respective ecosystems. If "least surprise" is a design goal, making pipx consistent with those would be my choice.

This is a really good thought. Unfortunately it seems that that each one does it a different way, one the way I prefer, the other one like @cs01 prefers.
npm uses the method that separates the command with and the arguments with a --:

npm run-script <command> [--silent] [-- <args>...]

wheras bundler sends command and all following arguments after it unedited to the command. Even if the first argument after command is --. (I just tested it to be sure)

bundle exec [--keep-file-descriptors] command

[clbarnes]

Perfect <_<

If in doubt, I'd go with npm's behaviour as a tiebreaker, as it's probably more widely used. Familiarity over purity.

[suominen]

The double dash is a guideline set by POSIX in IEEE Std 1003.1, Chapter 12: Utility Conventions, subchapter 12.2: Utility Syntax Guidelines, Guideline 10:

The argument -- should be accepted as a delimiter indicating the end of options. Any following arguments should be treated as operands, even if they begin with the '-' character. The -- argument should not be used as an option or as an operand.

argparse processes the first double dash and treats any remaining arguments as arguments (as opposed to options), including any additional double dashes.

See also:

• When and how was the double dash introduced as an end of options delimiter in Unix/Linux
• argparse: use of double dash to separate options and positional arguments (best to read the whole thread)

[itsayellow]

The double dash is a guideline set by POSIX in IEEE Std 1003.1, Chapter 12: Utility Conventions, subchapter 12.2: Utility Syntax Guidelines, Guideline 10:

I understand about the double-dash, I think the discussion was more about whether our doc guidance should position the double-dash between the command to run and the list of its arguments, OR whether it should should precede both.

[cs01]

think the discussion was more about whether our doc guidance should position the double-dash between the command to run and the list of its arguments, OR whether it should should precede both.

It should precede both.

If anything I would rather get rid of pipx's support for working without the --, similar to how nox does it (I actually added support in nox for this behavior, https://github.com/theacodes/nox/pull/229/files).

[cs01]

What about writing a note to stderr telling the user that the -- in this case is not being passed to pyapp? This note would be written only in cases where the next item in the argv list after app is a --. I think it might immediately help solve user confusion such as in the above example.

Sounds perfect!

[itsayellow]

The summary of my motivation here is:

There is a collection of arguments passed to and validated by pipx (app is part of these arguments).

There is a different collection of arguments forwarded, completely unparsed and untouched, to the app.

Those two collections of arguments should be separated explicitly. The best mechanism and convention for this is with the -- delimiter since

The argument -- should be accepted as a delimiter indicating the end of options.

I understand your motivation, I'm just worried it's different than expected for users accustomed to the usual unix CLI behavior of --. In my mind an "option" argument is anything that is optional (i.e. starts with a - or -- or an argument of such an option). Whereas in pipx run, app is positional and required.

If I'm belaboring this a little, it's because it sounds like the next release of pipx my be a major API-changing one and it seems now is the time to discuss this.

I always think of app and appargs being tightly coupled, as one command invocation that is run inside of a temporary pipx environment. I'm wondering if other users think the same way.

One very easy solution would to make app_and_appargs all one argument, giving it the nargs=argparse.REMAINDER argument. That way you could still precede it with -- if you want to, but it wouldn't change anything in-between app and appargs.

But I'll defer to @cs01 if this is just not the way you want to go. 🙂

[cs01]

Could you remind me if you are advocating for requiring -- every time, or only requiring that its position be immediately preceding the CLI app (or URL) to run?

[itsayellow]

I'd be up for not requiring anything. I'm requesting that the -- only be removed if it comes before all positional arguments, but not be removed if it comes between the positional app and appargs arguments.

Because as @suominen notes, it's not the usual unix CLI behavior for a -- coming after a positional (non-option) argument to be removed.

My solution would be to replace app and appargs with something equivalent to app_and_appargs that has the nargs=argparse.remainder set. That way everything including and after app would be passed on verbatim.

[cs01]

One very easy solution would to make app_and_appargs all one argument, giving it the nargs=argparse.REMAINDER argument. That way you could still precede it with -- if you want to, but it wouldn't change anything in-between app and appargs.

This sounds good to me, let's do it.

[cs01]

Note that pipx does support the case of a URL being the app, but I don't think that should be unworkable with the solution you proposed.

[cs01]

LMK when this is ready for review 👍

[itsayellow]

I've summarized our current test suite for pipx run argument parsing in the table below. Just to make sure we're all on the same page.

@suominen, could you take a look at it and see if the arguments are being parsed as you expect?

pipx command	app and appargs to execute
pipx run -- pycowsay -- hello	pycowsay -- hello
pipx run -- pycowsay -- -- hello	pycowsay -- -- hello
pipx run -- pycowsay hello --	pycowsay hello --
pipx run -- pycowsay hello -- --	pycowsay hello -- --
pipx run -- pycowsay --	pycowsay --
pipx run -- pycowsay -- --	pycowsay -- --
pipx run pycowsay -- hello	pycowsay -- hello
pipx run pycowsay -- -- hello	pycowsay -- -- hello
pipx run pycowsay hello --	pycowsay hello --
pipx run pycowsay hello -- --	pycowsay hello -- --
pipx run pycowsay --	pycowsay --
pipx run pycowsay -- --	pycowsay -- --
pipx run -- -- pycowsay --	-- pycowsay --

[itsayellow]

If we all like the way -- is being parsed then I think this is ready to go.

AFAIK, the only changes compared to current release pipx run behavior are:

• the usage info for pipx run has changed, instead of app and appargs there is now only a combinedapp ... with one line of help text.
• Now, if you put a -- directly after app, it will be passed to the app as the first apparg.

[suominen]

LGTM -- thank you!

[cs01]

@itsayellow you rock 🤘

Looks good besides the Windows CI failure (which is unrelated to your changes). Created #296. I am fine merging this before the Windows CI issue is fixed.