Skip to content

gh-131524: Update platform CLI to use argparse #131542

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

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

Conversation

Harry-Lees
Copy link
Contributor

@Harry-Lees Harry-Lees commented Mar 21, 2025

Closes: #131524
Related To: #131178

This PR updates the platform CLI to use argparse which adds --help flags and a usage section which was previously unavailable.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

And don't forget about a NEWS entry :)

@sobolevn sobolevn requested a review from hugovk March 21, 2025 17:43
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

We now have help which shows the arguments:

./python.exe -m platform -h
usage: python.exe -m platform [-h] [--terse] [--nonaliased] [{nonaliased,terse} ...]

positional arguments:
  {nonaliased,terse}

options:
  -h, --help          show this help message and exit
  --terse
  --nonaliased

But doesn't say what they do. Please can you add short descriptions?

Harry-Lees and others added 4 commits March 21, 2025 22:41
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@Harry-Lees
Copy link
Contributor Author

But doesn't say what they do. Please can you add short descriptions?

The help message now gives a short description of each flag

usage: python.exe -m platform [-h] [--terse] [--nonaliased] [{nonaliased,terse} ...]

positional arguments:
  {nonaliased,terse}

options:
  -h, --help          show this help message and exit
  --terse             return only the absolute minimum information needed to identify the platform
  --nonaliased        disable system/ OS name aliasing. If aliasing is enabled, some platforms will report system names which differ from their common names, e.g. SunOS will be reported
                      as Solaris

@Harry-Lees Harry-Lees requested a review from hugovk March 21, 2025 23:11
Lib/platform.py Outdated
Comment on lines 1467 to 1491
def _parse_args(args: list[str] | None):
import argparse

parser = argparse.ArgumentParser()
parser.add_argument("args", nargs="*", choices=["nonaliased", "terse"])
parser.add_argument(
"--terse",
action="store_true",
help=("return only the absolute minimum information needed to identify the "
"platform"),
)
parser.add_argument(
"--nonaliased",
dest="aliased",
action="store_false",
help=("disable system/ OS name aliasing. If aliasing is enabled, some "
"platforms will report system names which differ from their common "
"names, e.g. SunOS will be reported as Solaris"),
)

return parser.parse_args(args)


def _main(args: list[str] | None = None):
args = _parse_args(args)
Copy link
Member

@picnixz picnixz Mar 23, 2025

Choose a reason for hiding this comment

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

Suggested change
def _parse_args(args: list[str] | None):
import argparse
parser = argparse.ArgumentParser()
parser.add_argument("args", nargs="*", choices=["nonaliased", "terse"])
parser.add_argument(
"--terse",
action="store_true",
help=("return only the absolute minimum information needed to identify the "
"platform"),
)
parser.add_argument(
"--nonaliased",
dest="aliased",
action="store_false",
help=("disable system/ OS name aliasing. If aliasing is enabled, some "
"platforms will report system names which differ from their common "
"names, e.g. SunOS will be reported as Solaris"),
)
return parser.parse_args(args)
def _main(args: list[str] | None = None):
args = _parse_args(args)
def _make_parser():
import argparse
parser = argparse.ArgumentParser()
parser.add_argument("args", nargs="*", choices=["nonaliased", "terse"])
parser.add_argument(
"--terse",
action="store_true",
help=("return only the absolute minimum information needed "
"to identify platform"),
)
parser.add_argument(
"--nonaliased",
dest="aliased",
action="store_false",
help=("disable system/OS name aliasing. If aliasing is enabled, "
"some platforms will report system names which differ from "
"their common names, e.g., SunOS is reported as Solaris"),
)
return parser
def _main(args=None):
parser = _make_parser()
args = parser.parse(args)

Copy link
Member

Choose a reason for hiding this comment

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

Delete the space after the slash "system/ OS" -> "system/OS".

And this help is very long, can we make it shorter?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, but I just suggested to actually decouple the parsing from the creation of the argument parser (and reflowed the strings).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this help is very long, can we make it shorter?

I did think this while I was writing it, the second part is an excerpt from the platform.platform docs.

Just "disable system/OS name aliasing" was confusing to me, so I wanted to add some more information. The CLI itself is obviously just an interface for platform.platform(), I'm not sure if its appropriate to have the CLI say something like "See platform.platform docs for more information" or if theres any precedent for that in other modules.

I looked around some of the other CLI tools for similar help commands. Several other tools such as compileall, doctest, random, and trace have quite long help commands for arguments. In my opinion, I'd prefer a more descriptive than terse help message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just suggested to actually decouple the parsing from the creation of the argument parser

Do you mind if I ask what the reasoning behind this suggestion is? I'm happy to many any suggested refactors to the PR, this is not my codebase :D, but I haven't seen this pattern used anywhere, and quite a few of the other cPython CLI modules use the _main() and _parse_args() pattern. Are there any benefits to separating the stages like this?

I'll update the help message formatting though, I do think it reads nicer in your example :).

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer to use separate _main() and _parse_args(), it makes it easier to test separate parts of the CLI, following the example of https://pythontest.com/testing-argparse-apps/

And it will ease maintenance to use a similar pattern in different modules in this codebase.

Harry-Lees and others added 5 commits March 23, 2025 12:42
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Comment on lines +1482 to +1484
help=("disable system/OS name aliasing. If aliasing is enabled, "
"some platforms will report system names which differ from "
"their common names, e.g. SunOS will be reported as Solaris"),
Copy link
Member

Choose a reason for hiding this comment

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

Slightly shorter, with align:

Suggested change
help=("disable system/OS name aliasing. If aliasing is enabled, "
"some platforms will report system names which differ from "
"their common names, e.g. SunOS will be reported as Solaris"),
help=("disable system/OS name aliasing. If aliasing is enabled, "
"some platforms will report system names which differ from "
"their common names, e.g. SunOS will be reported as Solaris"),

Or if using Black style above:

Suggested change
help=("disable system/OS name aliasing. If aliasing is enabled, "
"some platforms will report system names which differ from "
"their common names, e.g. SunOS will be reported as Solaris"),
help=(
"disable system/OS name aliasing. If aliasing is enabled, "
"some platforms report system names different from "
"their common names, e.g. SunOS is reported as Solaris"
),



if __name__ == "__main__":
_main()
sys.exit(0)
Copy link
Member

Choose a reason for hiding this comment

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

I realise this was in the original, but looks redundant, especially with a single call under if __name__ == "__main__:

Suggested change
sys.exit(0)

Comment on lines +779 to +796
# This test tests that the arguments are correctly passed to the underlying
# `platform.platform()` call. The parameters are two booleans for `aliased`
# and `terse`.
options = (
(["--nonaliased"], (False, False)),
(["nonaliased"], (False, False)),
(["--terse"], (True, True)),
(["terse"], (True, True)),
(["nonaliased", "terse"], (False, True)),
(["--nonaliased", "terse"], (False, True)),
(["--terse", "nonaliased"], (False, True)),
)

for flags, args in options:
with self.subTest(flags=flags, args=args):
with mock.patch.object(platform, 'platform') as obj:
self.invoke_platform(*flags)
obj.assert_called_once_with(*args)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps like this, to document the parameters with variable names?

Suggested change
# This test tests that the arguments are correctly passed to the underlying
# `platform.platform()` call. The parameters are two booleans for `aliased`
# and `terse`.
options = (
(["--nonaliased"], (False, False)),
(["nonaliased"], (False, False)),
(["--terse"], (True, True)),
(["terse"], (True, True)),
(["nonaliased", "terse"], (False, True)),
(["--nonaliased", "terse"], (False, True)),
(["--terse", "nonaliased"], (False, True)),
)
for flags, args in options:
with self.subTest(flags=flags, args=args):
with mock.patch.object(platform, 'platform') as obj:
self.invoke_platform(*flags)
obj.assert_called_once_with(*args)
# Test that the arguments are correctly passed to the underlying
# `platform.platform()` call.
options = (
(["--nonaliased"], False, False),
(["nonaliased"], False, False),
(["--terse"], True, True),
(["terse"], True, True),
(["nonaliased", "terse"], False, True),
(["--nonaliased", "terse"], False, True),
(["--terse", "nonaliased"], False, True),
)
for flags, aliased, terse in options:
with self.subTest(flags=flags, aliased=aliased, terse=terse):
with mock.patch.object(platform, 'platform') as obj:
self.invoke_platform(*flags)
obj.assert_called_once_with(aliased, terse)

Comment on lines +776 to +777
# Due to backwards compatibility, the `aliased` and `terse` parameters
# are computed based on a combination of positional arguments and flags.
Copy link
Member

Choose a reason for hiding this comment

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

Remember to try and limit lines to 79 chars: https://peps.python.org/pep-0008/#maximum-line-length

Suggested change
# Due to backwards compatibility, the `aliased` and `terse` parameters
# are computed based on a combination of positional arguments and flags.
# For backwards compatibility, the `aliased` and `terse` parameters are
# computed based on a combination of positional arguments and flags.

Comment on lines +1475 to +1476
help=("return only the absolute minimum information needed to identify the "
"platform"),
Copy link
Member

Choose a reason for hiding this comment

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

Either align to opening delimiter:

Suggested change
help=("return only the absolute minimum information needed to identify the "
"platform"),
help=("return only the absolute minimum information needed "
"to identify the platform"),

Or I'd just go for Black style:

Suggested change
help=("return only the absolute minimum information needed to identify the "
"platform"),
help=(
"return only the absolute minimum information needed "
"to identify the platform"
),

Comment on lines +1482 to +1484
help=("disable system/OS name aliasing. If aliasing is enabled, "
"some platforms will report system names which differ from "
"their common names, e.g. SunOS will be reported as Solaris"),
Copy link
Member

Choose a reason for hiding this comment

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

Slightly shorter, with align:

Suggested change
help=("disable system/OS name aliasing. If aliasing is enabled, "
"some platforms will report system names which differ from "
"their common names, e.g. SunOS will be reported as Solaris"),
help=("disable system/OS name aliasing. If aliasing is enabled, "
"some platforms will report system names which differ from "
"their common names, e.g. SunOS will be reported as Solaris"),

Or if using Black style above:

Suggested change
help=("disable system/OS name aliasing. If aliasing is enabled, "
"some platforms will report system names which differ from "
"their common names, e.g. SunOS will be reported as Solaris"),
help=(
"disable system/OS name aliasing. If aliasing is enabled, "
"some platforms report system names different from "
"their common names, e.g. SunOS is reported as Solaris"
),

@donBarbos
Copy link
Contributor

Maybe it's worth adding a Command-line Usage section to the platform documentation?

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.

Improve platform CLI
5 participants