-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-133363: Fix Cmd completion for lines beginning with !
#133364
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
Conversation
When a line begins with `!` and there's no `do_shell` method defined, `parsecmd` returns `None` as the `cmd`, which incorrectly leads to `None` being concatenated to `complete_` and triggering a `TypeError`. Instead, recognize `None` as a sentinel that means we should call `completedefault`, as an empty string already is.
Lib/test/test_cmd.py
Outdated
|
||
output = run_pty(script, input) | ||
|
||
self.assertIn(b'ello', output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ello
because that's what is completed? hello
is not part of the output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, because that's what is used in the previous test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ello
because that's what's completed, yes. hello
is part of the output, but I'm just matching the test above that I copy-pasted from, which only checks for ab_completion_test
, heh
The tests do both pass if I adjust them to check for hello
and tab_completion_test
respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized I co-authored that test. Ha.
The :class:`cmd.Cmd` class has been fixed to call the ``completedefault`` | ||
method whenever the ``do_shell`` method is not defined and tab completion is | ||
requested for a line beginning with ``!``. Previously ``completedefault`` | ||
was called only if there were no spaces between the ``!`` and the cursor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously
completedefault
was called only if there were no spaces between the!
and the cursor position where tab completion was attempted.
Is that even true? I thought the completion never works:
elif line[0] == '!':
if hasattr(self, 'do_shell'):
line = 'shell ' + line[1:]
else:
return None, None, line
Anyway, normally in the news we only need to say what is fixed, we don't need to do a complete description of what's the previous behavior. News entry should be a concise sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that even true? I thought the completion never works:
Yes, because parsecmd
is only called if begidx > 0
, so in the !foo<Tab>
case (begidx=0 endidx=3) we don't hit this buggy codepath.
Anyway, normally in the news we only need to say what is fixed, we don't need to do a complete description of what's the previous behavior. News entry should be a concise sentence.
OK, I've shortened it and just say that it's fixed to reliably call the method, since previously it was inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay but it's still not good, because that's not really a name. It still won't give us the correct completion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not worry about that, we can do that in pdb I believe. I don't want to change cmd
so close to beta freeze. I'll loop back to this after beta freeze.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay but it's still not good, because that's not really a name. It still won't give us the correct completion.
True, though that seems to need a bigger change if you want to fix that... Do you want to special case !
at the start of the line, and do:
if begidx>0:
...
elif line.startswith("!"):
compfunc = self.completedefault
else:
compfunc = self.completenames
Even that's not enough to make PDB's completion work without the space, though, because that'll give a text="!h"
and PDB will say that none of the object names it knows of start with "!"
. Which means that making that work requires changes in the PDB module, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong. !
was by default a delimiter and that's why your current code passes. cmd
is handling this correctly because begidx
will >0
when you complete !h<tab>
. pdb
somehow removed !
from delimiters and I did not figure out why. However, we can deal with in pdb only, this is not a cmd
issue.
Lib/test/test_cmd.py
Outdated
""") | ||
|
||
# '! h' and complete 'ello' to '! hello' | ||
input = b"! h\t\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do add check here for !h\t\n
(without space) as well. No need for another method, just another input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does need to be another method, because the assertions are:
self.assertIn(b'hello', output)
self.assertIn(b'tab completion success', output)
and those would pass if either of the two inputs succeeded, not only if both of them did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we could use a subtest for this? I know how to avoid duplication with pytest, but I'm not really familiar with unittest. Does this work?
# '! h' or '!h' and complete 'ello' to 'hello'
for input in [b"! h\t\n", b"!h\t\n"]:
with self.subTest(input=input):
output = run_pty(script, input)
self.assertIn(b'hello', output)
self.assertIn(b'tab completion success', output)
Seems to...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean just use the same script and do two run_pty
- I don't think you even need a subtest. It's a very small case.
|
* origin/main: (111 commits) pythongh-91048: Add filename and line number to external inspection routines (pythonGH-133385) pythongh-131178: Add tests for `ast` command-line interface (python#133329) Regenerate pcbuild.sln in Visual Studio 2022 (python#133394) pythongh-133042: disable HACL* HMAC on Emscripten (python#133064) pythongh-133351: Fix remote PDB's multi-line block tab completion (python#133387) pythongh-109700: Improve stress tests for interpreter creation (pythonGH-109946) pythongh-81793: Skip tests for os.link() to symlink on Android (pythonGH-133388) pythongh-126835: Rename `ast_opt.c` to `ast_preprocess.c` and related stuff after moving const folding to the peephole optimizier (python#131830) pythongh-91048: Relax test_async_global_awaited_by to fix flakyness (python#133368) pythongh-132457: make staticmethod and classmethod generic (python#132460) pythongh-132805: annotationlib: Fix handling of non-constant values in FORWARDREF (python#132812) pythongh-132426: Add get_annotate_from_class_namespace replacing get_annotate_function (python#132490) pythongh-81793: Always call linkat() from os.link(), if available (pythonGH-132517) pythongh-122559: Synchronize C and Python implementation of the io module about pickling (pythonGH-122628) pythongh-69605: Add PyREPL import autocomplete feature to 'What's New' (python#133358) bpo-44172: Keep reference to original window in curses subwindow objects (pythonGH-26226) pythonGH-133231: Changes to executor management to support proposed `sys._jit` module (pythonGH-133287) pythongh-133363: Fix Cmd completion for lines beginning with `! ` (python#133364) pythongh-132983: Introduce `_zstd` bindings module (pythonGH-133027) pythonGH-91048: Add utils for printing the call stack for asyncio tasks (python#133284) ...
When a line begins with
!
and there's nodo_shell
method defined,parseline
returnsNone
as thecmd
, which incorrectly leads toNone
being concatenated tocomplete_
and triggering aTypeError
.Instead, recognize
None
as a sentinel that means we should callcompletedefault
, as an empty string already is.!
(bang space) #133363