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

Pylint no longer support separating options from positional arguments with -- #7003

Open
phst opened this issue Jun 22, 2022 · 11 comments · Fixed by #7096 or #7551
Open

Pylint no longer support separating options from positional arguments with -- #7003

phst opened this issue Jun 22, 2022 · 11 comments · Fixed by #7096 or #7551
Assignees
Labels
Configuration Related to configuration Needs PR This issue is accepted, sufficiently specified and now needs an implementation Regression

Comments

@phst
Copy link

phst commented Jun 22, 2022

Bug description

With Pylint 2.12, pylint -- file.py works as expected, with Pylint 2.14 it raises an error like "pylint: error: Unrecognized option found:". I guess that's related to the argparse migration.

Configuration

No response

Command used

pylint -- file.py

Pylint output

pylint: error: Unrecognized option found:

Expected behavior

The -- should separate options from positional arguments. In this case the invocation should have identical results to pylint file.py.

Pylint version

pylint 2.14.3
astroid 2.11.6
Python 3.9.12 (main, May  6 2022, 20:15:26) 
[GCC 11.2.0]

OS / Environment

Debian GNU/Linux

Additional dependencies

No response

@Pierre-Sassoulas
Copy link
Member

Thank you for opening the issue.

I added the discussion label because I don't know if we want to deviate from what argparse is doing and if it's worth it. In your example it's possible to use pylint file.py directly, right ? What is the standard we're talking about here, is it just something that optparse was doing or is it something well established for CLI ?

@phst
Copy link
Author

phst commented Jun 22, 2022

Thank you for opening the issue.

I added the discussion label because I don't know if we want to deviate from what argparse is doing and if it's worth it.

Argpase supports this syntax. From https://docs.python.org/3/library/argparse.html#arguments-containing:

you can insert the pseudo-argument '--' which tells parse_args() that everything after that is a positional argument

So this should work out of the box, and I'm wondering why it doesn't.

In your example it's possible to use pylint file.py directly, right ?

yes, the real use case is e.g. a script that wraps Pylint and receives arbitrary filenames, which might start with a dash.

What is the standard we're talking about here, is it just something that optparse was doing or is it something well established for CLI ?

It's quite established, see e.g. the GNU Coding Standards (https://www.gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html) or POSIX (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02, guideline 10).

@Pierre-Sassoulas Pierre-Sassoulas added this to To do: in Migration to argparse via automation Jun 22, 2022
@DanielNoord DanielNoord added the Configuration Related to configuration label Jun 22, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Jun 22, 2022
@DanielNoord DanielNoord self-assigned this Jun 30, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.5 milestone Jun 30, 2022
Migration to argparse automation moved this from To do: to Done Jun 30, 2022
phst added a commit to phst/rules_elisp that referenced this issue Aug 28, 2022
phst added a commit to phst/rules_elisp that referenced this issue Sep 17, 2022
@phst
Copy link
Author

phst commented Sep 17, 2022

I don't think this is fixed.

$ pylint --version
pylint 2.15.2
astroid 2.12.9
Python 3.10.6 (main, Aug 30 2022, 04:58:14) [Clang 13.1.6 (clang-1316.0.21.2.5)]

$ pylint -- build.py 
************* Module --
--:1: [F0001(fatal), ] No module named --

That is, Pylint now thinks that it should lint a file named --. Instead, it should skip the -- argument.

@DanielNoord
Copy link
Collaborator

Hm, that's weird. My test might not be good enough.

Putting this on my TODO list.

@DanielNoord DanielNoord reopened this Sep 17, 2022
Migration to argparse automation moved this from Done to To do: Sep 17, 2022
@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas Based on previous knowledge I think this should be a quick fix. If possible I'd like to get this into the patch release as well. I'll tell you after fixing the open PR if I can get a quick PR in for this as well!

@DanielNoord
Copy link
Collaborator

I[m going to look into another solution which will take some more time. Moving this from the milestone.

@DanielNoord
Copy link
Collaborator

@phst #7551 should fix this correctly now. If you want you can check that branch out and see if it works locally for you!

Migration to argparse automation moved this from To do: to Done Oct 2, 2022
@phst
Copy link
Author

phst commented Oct 2, 2022

Thanks but this still doesn't work. The purpose of the -- separator is to pass positional arguments that start with dashes and not have them confused with options. This doesn't work any more:

$ pip3 install https://github.com/PyCQA/pylint/archive/refs/heads/main.zip
[...]
$ touch -- -test.py
$ pylint -- -test.py
usage: pylint [options]
pylint: error: Unrecognized option found: test.py

@DanielNoord
Copy link
Collaborator

Can you give an example of how you would actually use this? The issue is that pylint currently accepts "files" to be scattered around the command line invocation.
So pylint file1.py --an-option file2.py --another-option=yes file3.py works. We do so be some special code that comes after the argparse parsing of arguments. This breaks your requested behaviour, but I'm not sure we can support both patterns. A good example will help me write an actually useful test to fix this.

@phst
Copy link
Author

phst commented Oct 2, 2022

Can you give an example of how you would actually use this?

The concrete case is https://github.com/phst/rules_elisp/blob/b79e86bc6adfdbba2576129f0a48e120afccd1c3/check_python.py#L83, but the general thinking is that wrapper scripts in general can't and shouldn't know whether filenames start with a dash or not. For example, a wrapper script like

files=(...)
pylint --foo=bar -- "${files[@]}"

shouldn't have to parse the argument list passed by the user or fail with weird errors (or worse, silently pass undesired options) if one of the files happens to start with a dash. One reason for the -- convention is to provide for an "obviously correct" syntax where the role of each argument is already clear from the syntax.

The issue is that pylint currently accepts "files" to be scattered around the command line invocation.

Maybe I'm missing something, but doesn't ArgumentParser.parse_intermixed_args do exactly that?

>>> p = argparse.ArgumentParser()
>>> p.add_argument('files', nargs='+')
>>> p.add_argument('--an-option', action='store_true')
>>> p.add_argument('--another-option')
>>> p.parse_intermixed_args('file1.py --an-option file2.py --another-option=yes file3.py -- -test.py'.split())
Namespace(an_option=True, another_option='yes', files=['file1.py', 'file2.py', 'file3.py', '-test.py'])

@DanielNoord
Copy link
Collaborator

The issue is that we don't have the files argument. So we can't collect nargs='+' into it. That is also why argparse doesn't solve this automatically for us: we parse with parse_known_args but it doesn't know to collect -test.py into a files argument.

I'm not sure we can add the files argument with nargs without it breaking other workflows. #7496 is a hack around it, but it still doesn't collect nargs.

@DanielNoord DanielNoord reopened this Oct 10, 2022
Migration to argparse automation moved this from Done to To do: Oct 10, 2022
@DanielNoord DanielNoord modified the milestones: 2.15.4, 2.16.0 Oct 10, 2022
phst added a commit to phst/rules_elisp that referenced this issue Nov 5, 2022
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.16.0, 2.17.0 Jan 8, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.17.0, 3.0.0 Mar 7, 2023
@Pierre-Sassoulas Pierre-Sassoulas removed this from the 3.0.0 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Related to configuration Needs PR This issue is accepted, sufficiently specified and now needs an implementation Regression
Projects
No open projects
3 participants