Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
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 forab_completion_test, heh
The tests do both pass if I adjust them to check forhello andtab_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 |
gaogaotiantianMay 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
completedefaultwas 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:
elifline[0]=='!':ifhasattr(self,'do_shell'):line='shell '+line[1:]else:returnNone,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, becauseparsecmd is only called ifbegidx > 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 toreliably 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 changecmd 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:
ifbegidx>0: ...elifline.startswith("!"):compfunc=self.completedefaultelse:compfunc=self.completenames
Even that's not enough to make PDB's completion work without the space, though, because that'll give atext="!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 becausebegidx 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 acmd 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 ifeither 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'forinputin [b"! h\t\n",b"!h\t\n"]:withself.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 tworun_pty - I don't think you even need a subtest. It's a very small case.
1d9406e intopython:mainUh oh!
There was an error while loading.Please reload this page.
bedevere-bot commentedMay 4, 2025
|
* 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) ...
Uh oh!
There was an error while loading.Please reload this page.
When a line begins with
!and there's nodo_shellmethod defined,parselinereturnsNoneas thecmd, which incorrectly leads toNonebeing concatenated tocomplete_and triggering aTypeError.Instead, recognize
Noneas a sentinel that means we should callcompletedefault, as an empty string already is.!(bang space) #133363