GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-103464: Add checks for arguments of pdb commands by gaogaotiantian · Pull Request #103465 · python/cpython Conversation
|
| count = 0 | ||
| elif len(args) == 2: | ||
| try: | ||
| count = int(args[1]) |
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, thanks for explaining
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| # 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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....
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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...
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Brandt Bucher <[email protected]>
|
Seems like some tests were failing but they did not seem related. Any idea why @brandtbucher ? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.