Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-103464: Add checks for arguments of pdb commands#103465
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
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 usesargs[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, whichint()
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 whenarg
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 callsrepr()
forarg
and asarg
isstr
, 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 aboutrepr(str)
returns indentical result, don't know what I was thinking.repr()
actually returns a worse result:
(Pdb)r'"'***Invalidargument:'\'"\''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 userepr
to add quotes. Didn't realize that there was additional escaping happening. The motivation here is to makearg
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:
Uh oh!
There was an error while loading.Please reload this page.
Misc/NEWS.d/next/Library/2023-04-12-03-03-27.gh-issue-103464.Oa_8IW.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Seems like some tests were failing but they did not seem related. Any idea why@brandtbucher ? |
Uh oh!
There was an error while loading.Please reload this page.