Skip to content

Conversation

@SengerM
Copy link

@SengerM SengerM commented Aug 23, 2021

Fixed bug with date formats like %Y-%m-%d in help messages of argparse

Now this code does not produce any error when invoked with the -h option and produces the expected result:

import argparse

parser = argparse.ArgumentParser(description = 'Just a test code.')
parser.add_argument(
	'--from',
	metavar = 'when', 
	help = 	'A date in format %Y-%m-%d',
	dest = f'From',
	type = str,
	required = True,
)
args = parser.parse_args()

print(args.From)

https://bugs.python.org/issue44986

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@SengerM

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@SengerM
Copy link
Author

SengerM commented Aug 24, 2021

I don't know if a human will ever read this, but if so I have already signed the CLA yesterday and in the check yourself page still says that I have not signed it. Is it normal that it takes so long?

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Oct 1, 2021
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Choose a reason for hiding this comment

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

Ran the argparse test module:
Ran 1670 tests in 3.185s
OK
== Tests result: SUCCESS ==

And example in bug ok.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 12, 2022
@furkanonder
Copy link
Contributor

@SengerM Can you add the unit tests for this case?

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@SengerM
Copy link
Author

SengerM commented Aug 23, 2023

Dear @furkanonder , sorry for the long delay. I added the test case. I see there are fails in the tests for macOS, but they don't seem related to my changes.

@SengerM SengerM requested a review from rhettinger August 23, 2023 08:43
@rhettinger rhettinger removed their request for review August 23, 2023 23:16
Copy link
Member

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

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

I know this PR has been sitting for a little while, but I pulled it down and rebased it with the main, and it still resolves the bug. Just a couple of comments but overall, this LGTM.

return self._get_help_string(action) % params
expanded_help = self._get_help_string(action)
for param, value in params.items():
if f'%({param})s' in expanded_help:
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 this conditional is redundant, right? Doesn't replace() handle validating that the string is contained by expanded_help?

version = ''

class TestHelpWithPercentageSymbols(HelpTestCase):
"""Test a help message including % symbols"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Test a help message including % symbols"""
"""Test help output that includes % symbols"""


class TestHelpWithPercentageSymbols(HelpTestCase):
"""Test a help message including % symbols"""
parser_signature = Sig(prog='PROG', description='Just a test code.')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
parser_signature = Sig(prog='PROG', description='Just a test code.')
parser_signature = Sig(prog='PROG', description='Just a test program.')

parser_signature = Sig(prog='PROG', description='Just a test code.')
argument_signatures = [
Sig('--somearg', metavar='somearg',
help='Now you dont need to escape this: %, and you will not get nonsensical errors!'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
help='Now you dont need to escape this: %, and you will not get nonsensical errors!'),
help='No escape required for %'),

argument_signatures = [
Sig('--somearg', metavar='somearg',
help='Now you dont need to escape this: %, and you will not get nonsensical errors!'),
Sig('--date', metavar='when', help='A date in format %Y-%m-%d',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Sig('--date', metavar='when', help='A date in format %Y-%m-%d',
Sig('--date', metavar='when', help='A date in %Y-%m-%d format',

Sig('--date', metavar='when', help='A date in format %Y-%m-%d',
dest=f'date', type=str, required=True),
Sig('bar', nargs='?', type=int, default=42,
help='the bar to %(prog)s (default: %(default)s)'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
help='the bar to %(prog)s (default: %(default)s)'),
help='The bar to %(prog)s (default: %(default)s)'),

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.

It's not that simple.

What if the replacement value contains %% or %(prog)s? Or only the part of it, and the other part is contained in the help string? E.g. help='%(default)s(prog)s', where default='%'.

What if the help string contains %(...)r or %(...)a instead of %(...)s?

@bedevere-app
Copy link

bedevere-app bot commented Sep 24, 2024

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.

@serhiy-storchaka
Copy link
Member

argparse now checks help string for correctness, so you will be informed about errors earlier.

You can implement a "safe" help formatter as a separate class, but this is not easy task. I am not sure that argparse needs this. It is easier to fix help strings in the user code.

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.

9 participants