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

gh-91818: Add executable detection for clis #91819

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

scrummyin
Copy link
Contributor

@scrummyin scrummyin commented Apr 22, 2022

The help message for things like json.tool and ast should match the
executable that was run, for example python.exe on windows and python3 on
*nix OSes. This will facilitate users being able to copy and paste from
the help text displayed by argparse, and clarify which version of python
is being run on some OSes.

The previous display for json.tool of

$ python3 -m json.tool -h
usage: python -m json.tool [-h] [--sort-keys] [--json-lines] [infile] [outfile]

becomes

$ python3 -m json.tool -h
usage: python3 -m json.tool [-h] [--sort-keys] [--json-lines] [infile] [outfile]

Pip has a something like PrettyExecutableName in a funciton called
get_prog found in
https://github.com/pypa/pip/blob/main/src/pip/_internal/utils/misc.py.
The pip version has a lot of defensive coding practices around catching
various exception related to sys.argv. But since this version does not
parse argv, those exception can be dropped.

As a side effect the help usage of venv has been updated to use the
python -m syntax as it was the only command not using that
style, and docs show that style of invocation.

The help message for things like json.tool and ast should match the
executable that was run, for example python.exe on windows and python3 on
*nix OSes. This will facilitate users being able to copy and paste from
the help text displayed by argparse, and clarify which version of python
is being run on some OSes.

The previous display for json.tool of
```
$ python3 -m json.tool -h
usage: python -m json.tool [-h] [--sort-keys] [--json-lines] [infile] [outfile]
```

becomes
```
$ python3 -m json.tool -h
usage: python3 -m json.tool [-h] [--sort-keys] [--json-lines] [infile] [outfile]
```

Pip has a something like PrettyExecutableName in a funciton called
get_prog found in
https://github.com/pypa/pip/blob/main/src/pip/_internal/utils/misc.py.
The pip version has a lot of defensive coding practices around catching
various exception related to sys.argv. But since this version does not
parse argv, those exception can be dropped.

As a side effect the help usage of venv has been updated to use the
python -m <module> syntax as it was the only command not using that
style, and docs show that style of invocation.
@scrummyin scrummyin changed the title gn-91818: Add executable detection for clis gh-91818: Add executable detection for clis Apr 22, 2022
@scrummyin
Copy link
Contributor Author

@scrummyin scrummyin commented Apr 22, 2022

I'm happy to update the news if people think this is a good idea. Also as a side note, the Ubuntu tests failed on me before for no reason, and that is odd since this was written and originally tested on Ubuntu. So any other OS would not have been a surprise, but the only one I know the tests run on failing is.

@vsajip
Copy link
Member

@vsajip vsajip commented Apr 25, 2022

As a required test is still failing, you could try rebasing so that the latest changes in main are reflected in this PR.

Copy link
Member

@vsajip vsajip left a comment

Please see my suggestions.

@@ -56,6 +56,9 @@
ArgumentDefaultsHelpFormatter adds information about argument defaults
to the help.
- PrettyExecutableName -- A class to create strings for use as the
Copy link
Member

@vsajip vsajip Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a class seems overkill, see below.

@@ -2597,3 +2601,24 @@ def error(self, message):
self.print_usage(_sys.stderr)
args = {'prog': self.prog, 'message': message}
self.exit(2, _('%(prog)s: error: %(message)s\n') % args)


class PrettyExecutableName(object):
Copy link
Member

@vsajip vsajip Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a class when a function will do? For example, this function:

def adjust_command(cmd):
    """
    Adjust a command line which uses 'python' to substitute the name
    of the actual running interpreter, e.g. 'python3' or 'python3.10'.
    """
    parts = cmd.split(' ', 1)
    exe = parts[0]
    if exe != 'python':
        return cmd
    if exe == 'python':
        sysexe = sys.executable
        if not sysexe:
            return cmd
        exe = os.path.basename(sysexe)
        if os.name == 'nt':
            exe = os.path.splitext(exe)[0]
    return exe if len(parts) == 1 else f'{exe} {parts[1]}'

would appear to do the trick and not be restricted to e.g. -m commands. You should be able to try it interactively here.

@@ -1699,7 +1699,8 @@ def unparse(ast_obj):
def main():
import argparse

parser = argparse.ArgumentParser(prog='python -m ast')
program_name = argparse.PrettyExecutableName("ast")
parser = argparse.ArgumentParser(prog=f"{program_name}")
Copy link
Member

@vsajip vsajip Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With my suggestion this would become simpler. e.g.

parser = argparse.ArgumentParser(prog=argparse.adjust_command('python -m ast'))

which is also clearer than hiding the python -m in the function, as you've done in your implementation.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 26, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

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

Successfully merging this pull request may close these issues.

None yet

3 participants