Skip to content

gh-129911: pygettext: Fix the keyword entry in help output #129914

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

Merged
merged 3 commits into from
Feb 14, 2025

Conversation

tomasr8
Copy link
Member

@tomasr8 tomasr8 commented Feb 9, 2025

This simply replaces the interpolated value %(DEFAULTKEYWORDS)s with the list of default keywords.
The default keywords are unlikely to change often so there is no advantage to keeping the help text parametrized.

Copy link
Contributor

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

Why hard code them? We can just get the keywords from the dictionary.

__doc__ = __doc__ % {"DEFAULTKEYWORDS": ", ".join(DEFAULTKEYWORDS.keys())}
--- pygettext.py	2025-02-09 21:26:04.337335656 +0000
+++ pygettext-proposed.py	2025-02-09 21:27:36.552807306 +0000
@@ -171,7 +171,7 @@
 
 
 def usage(code, msg=''):
-    print(__doc__ % globals(), file=sys.stderr)
+    print(__doc__, file=sys.stderr)
     if msg:
         print(msg, file=sys.stderr)
     sys.exit(code)
@@ -295,6 +295,7 @@
     'dnpgettext': {1: 'msgctxt', 2: 'msgid', 3: 'msgid_plural'},
 }
 
+__doc__ = __doc__ % {"DEFAULTKEYWORDS": ", ".join(DEFAULTKEYWORDS.keys())}
 
 def matches_spec(message, spec):
     """Check if a message has all the keys defined by the keyword spec."""

@tomasr8
Copy link
Member Author

tomasr8 commented Feb 9, 2025

Why hard code them?

Good question! Several reasons:

  • The default keyword list is not going to change anytime soon, so we don't gain much by keeping it dynamic.
  • Having to format a module string feels like a strange pattern so I prefer to avoid it and rather keep it simple.
  • If the default keyword format changes again (which is more likely than adding a new default keyword), we might get some ugly output and we'll have to fix the help text yet again (same reason for this PR existing).

Copy link
Contributor

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

Good enough explanation for me

"""Test that the help text is displayed."""
res = assert_python_ok(self.script, '--help')
self.assertEqual(res.out, b'')
self.assertIn(b'pygettext -- Python equivalent of xgettext(1)', res.err)
Copy link
Member Author

Choose a reason for hiding this comment

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

I only test that the --help argument works, not that the help text matches a snapshot exactly, though I can add it

@tomasr8
Copy link
Member Author

tomasr8 commented Feb 13, 2025

@AA-Turner is it ok if I request your review on PRs related to pygettext? I don't want to create too much noise if you'd prefer not to 🙂

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@serhiy-storchaka
Copy link
Member

Note also the output of ./python -m pydoc Tools/i18n/pygettext.py.

Using __doc__ as the help in getopt-based scripts is a nice trick. But there are issues with dynamically generated help and localization. Hardcoding the list of names looks acceptable in this context.

@serhiy-storchaka serhiy-storchaka merged commit 9d1e668 into python:main Feb 14, 2025
43 checks passed
@tomasr8 tomasr8 deleted the pygettext-help branch February 14, 2025 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants