Skip to content

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.

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

Merged
merged 4 commits into from
May 4, 2025
Merged

gh-133363: Fix Cmd completion for lines beginning with ! #133364

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12e2495
Fix Cmd completion for lines beginning with `! `
godlygeek May 4, 2025
973bef6
Check falsiness instead
godlygeek May 4, 2025
1c6248e
Make news entry less descriptive
godlygeek May 4, 2025
c259bef
Test with and without a space after !
godlygeek May 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Lib/cmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ def complete(self, text, state):
endidx = readline.get_endidx() - stripped
if begidx>0:
cmd, args, foo = self.parseline(line)
if cmd == '':
if not cmd:
compfunc = self.completedefault
else:
try:
Expand Down
25 changes: 25 additions & 0 deletions Lib/test/test_cmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,31 @@ def do_tab_completion_test(self, args):
self.assertIn(b'ab_completion_test', output)
self.assertIn(b'tab completion success', output)

def test_bang_completion_without_do_shell(self):
script = textwrap.dedent("""
import cmd
class simplecmd(cmd.Cmd):
def completedefault(self, text, line, begidx, endidx):
return ["hello"]

def default(self, line):
if line == "! hello":
print('tab completion success')
else:
print('tab completion failure')
return True

simplecmd().cmdloop()
""")

# '! h' and complete 'ello' to '! hello'
input = b"! h\t\n"
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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...

Copy link
Member

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.


output = run_pty(script, input)

self.assertIn(b'ello', output)
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

@godlygeek godlygeek May 4, 2025 •
edited
Loading

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.

Copy link
Member

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.

self.assertIn(b'tab completion success', output)

def load_tests(loader, tests, pattern):
tests.addTest(doctest.DocTestSuite())
return tests
Expand Down
5 changes: 5 additions & 0 deletions Misc/NEWS.d/next/Library/2025-05-03-21-55-33.gh-issue-133363.PTLnRP.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
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
Copy link
Member

@gaogaotiantian gaogaotiantian May 4, 2025 •
edited
Loading

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.

Copy link
Contributor Author

@godlygeek godlygeek May 4, 2025 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

@godlygeek godlygeek May 4, 2025 •
edited
Loading

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.

Copy link
Member

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.

position where tab completion was attempted.
Loading