-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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
gh-103464: Add checks for arguments of pdb commands #103465
Conversation
gaogaotiantian
commented
Apr 12, 2023
•
edited by bedevere-bot
Loading
edited by bedevere-bot
- Issue: Add argument checking on pdb commands #103464
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.
Great work. I played with this a bit and found it very useful.
Also added some minor comments and an alternative to inspecting the frame of caller for docs
count = 0 | ||
elif len(args) == 2: | ||
try: | ||
count = int(args[1]) |
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.
The current code uses args[1].strip()
.
Even though I don't have an example for when this would be useful, I suggest not to omit .strip()
here to maintain consistency with the rest of the code here.
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.
args[1].strip()
only strips spaces, which int()
takes care of when it tries to convert the string to integer. This is redundant so I removed it.
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.
Fair enough, thanks for explaining
Lib/pdb.py
Outdated
# Yes it's a bit hacky. Get the caller name, get the method based on | ||
# that name, and get the docstring from that method. | ||
# This should NOT fail if the caller is a method of this class. | ||
doc = inspect.getdoc(getattr(self, sys._getframe().f_back.f_code.co_name)) |
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.
To avoid this, the __doc__
of the caller could be passed in explicitly.
This would add some verbosity to the calls, but these are already identical in terms of the arguments.
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.
There's no clean way to pass the __doc__
of the caller inside the caller. I'd rather keep the ugliness in a single place and document it, than pollute everywhere. With this hack(which is not hard to understand as long as you are familiar with the frame/code structure), all the callers are very clean. Similar methods are actually used to get the docs for all the commands.
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 understand the motivation here. Both solutions are not ideal and if equally "unclean" pulling it in a single place does make sense. Whether this is the case here, may be just personal opinion....
def _print_invalid_arg(self, arg): | ||
"""Return the usage string for a function.""" | ||
|
||
self.error(f"Invalid argument: {arg}") |
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.
Maybe consider using {arg!r}
to avoid confusion when arg
contains spaces
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.
!r
simply calls repr()
for arg
and as arg
is str
, it should have the identical behavior. Not sure what you are trying to achieve here, could you give an example where !r
gives a different result?
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 I was wrong about repr(str)
returns indentical result, don't know what I was thinking. repr()
actually returns a worse result:
(Pdb) r '"'
*** Invalid argument: '\'"\''
Usage: r(eturn)
(Pdb)
{args}
prints exactly what user types and that is more intuitive.
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 usually use repr
to add quotes. Didn't realize that there was additional escaping happening. The motivation here is to make arg
appear as a single value. This really is just a nit, so...
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.
Actually, taking a look at the rest of the code, I realized people have different ideas about this. The current implementation contains a mix of %r
and %s
and different efforts to "contain" the argument with '
or ()
. Some, however, is untouched. It might be beneficial if we could unify them - %r
has the escaping side effect and we should use explicit quotes if possible.
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.
This is a really nice usability improvement. Thanks for implementing it!
Just two suggestions, then I'll land this once the feature freeze is lifted:
Misc/NEWS.d/next/Library/2023-04-12-03-03-27.gh-issue-103464.Oa_8IW.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Seems like some tests were failing but they did not seem related. Any idea why @brandtbucher ? |