GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-103464: Add checks for arguments of pdb commands by gaogaotiantian · Pull Request #103465 · python/cpython

Conversation

@gaogaotiantian

@gaogaotiantian gaogaotiantian commented Apr 12, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Add argument checking on pdb commands #103464

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gaogaotiantian
Add checks for command arguments
714f506
@bedevere-bot bedevere-bot mentioned this pull request Apr 12, 2023
Add argument checking on pdb commands #103464
Closed
@bedevere-bot bedevere-bot added the awaiting review label Apr 12, 2023
@blurb-it
📜🤖 Added by blurb_it.
d31568a
skoslowski
skoslowski reviewed Apr 24, 2023
View reviewed changes

@skoslowski skoslowski left a comment

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/pdb.py
count = 0
elif len(args) == 2:
try:
count = int(args[1])

@skoslowski skoslowski Apr 24, 2023

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@gaogaotiantian gaogaotiantian Apr 24, 2023

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@skoslowski skoslowski Apr 24, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread 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))

@skoslowski skoslowski Apr 24, 2023

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@gaogaotiantian gaogaotiantian Apr 24, 2023

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@skoslowski skoslowski Apr 24, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/pdb.py
def _print_invalid_arg(self, arg):
"""Return the usage string for a function."""

self.error(f"Invalid argument: {arg}")

@skoslowski skoslowski Apr 24, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@gaogaotiantian gaogaotiantian Apr 24, 2023

Copy link
Copy Markdown
Member Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@gaogaotiantian gaogaotiantian Apr 24, 2023

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@skoslowski skoslowski Apr 24, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@gaogaotiantian gaogaotiantian Apr 24, 2023

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@brandtbucher
brandtbucher self-requested a review April 28, 2023 17:49
brandtbucher
brandtbucher approved these changes May 10, 2023
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Misc/NEWS.d/next/Library/2023-04-12-03-03-27.gh-issue-103464.Oa_8IW.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels May 10, 2023
@brandtbucher brandtbucher self-assigned this May 10, 2023
@brandtbucher brandtbucher added the stdlib Standard Library Python modules in the Lib/ directory label May 10, 2023
gaogaotiantian and others added 2 commits May 11, 2023 08:58
@gaogaotiantian @brandtbucher
Use argument in sys._getframe to avoid f_back
1e541f8
Co-authored-by: Brandt Bucher <[email protected]>
@brandtbucher
Update NEWS entry
095150d
@brandtbucher
brandtbucher enabled auto-merge (squash) May 22, 2023 22:50
@gaogaotiantian

gaogaotiantian commented May 26, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher
Merge branch 'main' into pdb-arg-checking
5f70f5b
@brandtbucher
brandtbucher merged commit d944d87 into python:main May 31, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label May 31, 2023
@gaogaotiantian
gaogaotiantian deleted the pdb-arg-checking branch June 3, 2023 03:49
@gaogaotiantian gaogaotiantian mentioned this pull request Oct 26, 2023
Pdb bug when using names that collide w/ aliases of breakpoints operations #111352
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucher brandtbucher brandtbucher approved these changes
+1 more reviewer
@skoslowski skoslowski skoslowski left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@brandtbucher brandtbucher

Labels

stdlib Standard Library Python modules in the Lib/ directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@gaogaotiantian @skoslowski @brandtbucher @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.