gh-110944: Make pdb completion work for alias and convenience vars #110945
Conversation
gaogaotiantian
changed the title
| @@ -232,7 +232,7 @@ def __init__(self, completekey='tab', stdin=None, stdout=None, skip=None, | |||
| try: | |||
| import readline | |||
| # remove some common file name delimiters | |||
| readline.set_completer_delims(' \t\n`@#$%^&*()=+[{]}\\|;:\'",<>?') | |||
| readline.set_completer_delims(' \t\n`@#%^&*()=+[{]}\\|;:\'",<>?') | |||
There was a problem hiding this comment.
There are no tests in this PR.
What could this change break?
There was a problem hiding this comment.
There's no test for completion, presumably because it's not easy to test. It would change the behavior when the user tries to do a complete (hit <tab> in most cases) and there's a $ in the line. In that case, $ would be not considered as a delimiter (the text to be completed would include $). I can't think of any real cases where this matters. Actually, I can't think of a case where the user uses $ in their pdb commands (other than using convenience variable of course).
| @@ -670,6 +670,19 @@ def set_convenience_variable(self, frame, name, value): | |||
| # Generic completion functions. Individual complete_foo methods can be | |||
| # assigned below to one of these functions. | |||
|
|
|||
| def completenames(self, text, line, begidx, endidx): | |||
There was a problem hiding this comment.
This is overwritting the completenames method defined in cmd.Cmd, which is called when the user tries to complete the command.
Pdb class has some implicit inheritance from cmd.Cmd class to achieve customized behaviors. I can add some comments here if it helps.
There was a problem hiding this comment.
I think adding a test that covers this would be better.
There was a problem hiding this comment.
I don't think you can (easily) set up a readline test to really trigger the "complete" feature of readline. Even test_cmd does not test against complete features for cmd.Cmd. I can add some unitttest just to test this method (feeding mock arguments to the function), but a real functional test involving readline would be a large amount of work - and we should probably do that for cmd first if we consider adding it.
|
Would you be willing to add that test? It would make it much easier to review this change. There is an existing |
I put the helper into a new file as you said, then added some completion tests for pdb. It's not "complete", but it's at least something, and it tests the changes I made for this PR. I'll see if there are more interesting cases now that we have the structure. Also, |
|
Hi @iritkatriel , tests were added for both pdb and cmd. After we merge this, we can do the cmd libedit support. (Or we can do it vice versa, but writing test for the other PR is difficult). This PR:
The other PR:
So either
|
|
I'm not familiar with libedit/pty. I think @serhiy-storchaka might be? |
There was a problem hiding this comment.
Thank you. I'm not very familiar with libedit/pty either, but this looks reasonable to me. If we don't find a better expert, let's merge it!
It would be best if you could split this PR into one that adds the test_basic_completion test, and the one that adds the functionality (and the test for it).
The basic test could then be more easily backported to maintenance versions of Python. It's OK to link both PRs to the same issue.
I also have a few nitpicks below.
(If you don't want to take on the extra work, let me know -- I can take over)
Co-authored-by: Petr Viktorin <encukou@gmail.com>
|
@encukou I've applied the suggestions on the code left :) Let me know if there's anything missing from the PR. |
|
Looks good, thank you! |
$was removed from the delimiters as it was used for convenience variables.