Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
brandtbucher merged 5 commits intopython:mainfromgaogaotiantian:pdb-arg-checking
May 31, 2023

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantiangaogaotiantian commentedApr 12, 2023
edited by bedevere-bot
Loading

Copy link
Contributor

@skoslowskiskoslowski left a 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])
Copy link
Contributor

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.

Copy link
MemberAuthor

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.

Copy link
Contributor

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))
Copy link
Contributor

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.

Copy link
MemberAuthor

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.

Copy link
Contributor

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}")
Copy link
Contributor

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

Copy link
MemberAuthor

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?

Copy link
MemberAuthor

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.

Copy link
Contributor

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

Copy link
MemberAuthor

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.

@brandtbucherbrandtbucher self-requested a reviewApril 28, 2023 17:49
Copy link
Member

@brandtbucherbrandtbucher left a 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:

@brandtbucherbrandtbucher self-assigned thisMay 10, 2023
@brandtbucherbrandtbucher added the stdlibPython modules in the Lib dir labelMay 10, 2023
gaogaotiantianand others added2 commitsMay 11, 2023 08:58
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@brandtbucherbrandtbucherenabled auto-merge (squash)May 22, 2023 22:50
@gaogaotiantian
Copy link
MemberAuthor

Seems like some tests were failing but they did not seem related. Any idea why@brandtbucher ?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@skoslowskiskoslowskiskoslowski left review comments

@brandtbucherbrandtbucherbrandtbucher approved these changes

Assignees

@brandtbucherbrandtbucher

Labels
stdlibPython modules in the Lib dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@gaogaotiantian@skoslowski@brandtbucher@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp