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.

Sign up for GitHub

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

Jump to bottom

Argument Clinic 'destination <name> clear' is broken #106970

Closed
erlend-aasland opened this issue Jul 21, 2023 · 2 comments
Closed

Argument Clinic 'destination <name> clear' is broken #106970

erlend-aasland opened this issue Jul 21, 2023 · 2 comments
Assignees
@erlend-aasland
Labels
topic-argument-clinic type-bug An unexpected behavior, bug, or error

Comments

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jul 21, 2023 •
edited by bedevere-bot
Loading

The destination <name> clear command is documented like this:

It removes all the accumulated text up to this point in the destination. (I don’t know what you’d need this for, but I thought maybe it’d be useful while someone’s experimenting.)

Clearly, the last sentence is correct, because this command contains two bugs, and nobody has ever complained:

  1. It accesses the wrong attribute when clearing the accumulators
  2. The directive parser does not return immediately after executing the clear command, so it jumps straight into "fail because we got an unknown command"

The fixes are easy, so I suggest we apply the fixes and add regressions tests. An alternative could be to remove the clear command, but I don't think we should tear out features lightly, no matter how obscure they are.

Found while working on #106935.

Linked PRs
@erlend-aasland erlend-aasland added type-bug An unexpected behavior, bug, or error topic-argument-clinic labels Jul 21, 2023
@erlend-aasland erlend-aasland self-assigned this Jul 21, 2023
@bedevere-bot bedevere-bot mentioned this issue Jul 21, 2023
Merged
erlend-aasland added a commit that referenced this issue Jul 22, 2023
@erlend-aasland
gh-106970: Fix Argument Clinic 'destination <name> clear' command (#1…
3372bcb 
…06972)

Add test for the 'destination <name> clear' command,
and the 'destination' directive in general.

Fix two bugs in 'destination <name> clear' command:

1. The text attribute of the allocator is called 'text', not '_text'
2. Return after processing the 'clear' command,
   instead of proceeding directly to the fail().
@bedevere-bot bedevere-bot mentioned this issue Jul 22, 2023
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 22, 2023
@erlend-aasland @miss-islington
pythongh-106970: Fix Argument Clinic 'destination <name> clear' comma…
588f331 
…nd (pythonGH-106972)

Add test for the 'destination <name> clear' command,
and the 'destination' directive in general.

Fix two bugs in 'destination <name> clear' command:

1. The text attribute of the allocator is called 'text', not '_text'
2. Return after processing the 'clear' command,
   instead of proceeding directly to the fail().
(cherry picked from commit 3372bcb)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
erlend-aasland added a commit that referenced this issue Jul 22, 2023
@miss-islington @erlend-aasland
[3.12] gh-106970: Fix Argument Clinic 'destination <name> clear' comm…
67748f1 
…and (GH-106972) (#106983)

Add test for the 'destination <name> clear' command,
and the 'destination' directive in general.

Fix two bugs in 'destination <name> clear' command:

1. The text attribute of the allocator is called 'text', not '_text'
2. Return after processing the 'clear' command,
   instead of proceeding directly to the fail().
(cherry picked from commit 3372bcb)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
@bedevere-bot bedevere-bot mentioned this issue Jul 22, 2023
Merged
erlend-aasland added a commit that referenced this issue Jul 22, 2023
@erlend-aasland
[3.11] gh-106970: Fix Argument Clinic 'destination <name> clear' comm…
63c945a 
…and (#106972) (#107059)

Add test for the 'destination <name> clear' command,
and the 'destination' directive in general.

Fix two bugs in 'destination <name> clear' command:

1. The text attribute of the allocator is called 'text', not '_text'
2. Return after processing the 'clear' command,
   instead of proceeding directly to the fail().

(cherry picked from commit 3372bcb)
@AlexWaygood
Copy link
Member

Is this fixed now?

@erlend-aasland
Copy link
Contributor Author

Yep!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@erlend-aasland erlend-aasland

Labels
topic-argument-clinic type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@erlend-aasland @AlexWaygood