-
-
Notifications
You must be signed in to change notification settings - Fork 33k
bpo-33294: Support complex expressions for py-print command. #6481
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
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. When your account is ready, please add a comment in this pull request Thanks again to your contribution and we look forward to looking at it! |
I've just signed CLA, please recheck it. |
@marxin Please, open an issue in http://bugs.python.org and reference that issue number in the pull request title. More info about the workflow: |
bfcc0c3
to
5299070
Compare
@pablogsal Thanks. Done that, looks issue-number check needs to be triggered again. |
@marxin Reference the issue number in the PR title. See other PRs for reference. Thanks |
Thanks @pablogsal. Done that. |
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.
Thanks for this patch.
The patch is missing test coverage. Please see: Lib/test/test_gdb.py for examples of how to test py-print.
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.
Is this missing some line breaks? (i.e. before the "local")
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.
Yes, fixed in newer version.
Tools/gdb/libpython.py
Outdated
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.
Shouldn't this be a docstring i.e. within the function body?
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.
Yes, fixed in newer version.
Tools/gdb/libpython.py
Outdated
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.
It seems a bit odd to be using dotted notation to access by index. Is it possible to support square-bracket syntax here, or does it conflict with gdb's usage of square-bracket syntax for C level lookup?
Tools/gdb/libpython.py
Outdated
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.
Similarly for dict objects; wouldn't square bracket notation be more Pythonic?
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.
Yes, fixed in newer version.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
5299070
to
0180f67
Compare
Tools/gdb/libpython.py
Outdated
@classmethod | ||
def tokenize_square_brackets(self, expr): | ||
''' | ||
Tokenize single expression of an array index of key of a dictionary |
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 docstring is confusing to read, could you elaborate a cleaner version of what this function does?
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.
Should be better now ;)
0180f67
to
4b9980e
Compare
I also added a test-case. I have made the requested changes; please review again |
Thanks for making the requested changes! @davidmalcolm: please review the changes made to this pull request. |
May I please ping review process? |
Lib/test/test_gdb.py
Outdated
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 skills[defence]
syntax seems weird to me; shouldn't this be skills["defence"]
or skills['defence']
?
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.
Likewise here; shouldn't this be ['a']
rather than [a]
?
Tools/gdb/libpython.py
Outdated
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.
Please add test coverage for this error
Tools/gdb/libpython.py
Outdated
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.
Please add test coverage for this error.
Tools/gdb/libpython.py
Outdated
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.
Please add test coverage of a tuple
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.
Done.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
4b9980e
to
41c700a
Compare
Thanks @davidmalcolm for review. |
Thanks for making the requested changes! @davidmalcolm: please review the changes made to this pull request. |
I would like to remind review @davidmalcolm. Thanks. |
PING^2 |
PING^3 |
@davidmalcolm: Hi. Can we please move forward with this pull request? |
@davidmalcolm : ping |
@csabella Can you please find somebody else from David who can make the patch review? |
Bailing out, apparently, there's nobody who would review my patch. |
The patch is based on a blog post: http://kouk.surukle.me/2014/09/25/debugging-python-objects-and-fields-with-gdb/.
Adding him: @kouk
Purpose of the pull request is to support more complex expressions for py-print command.
Small example:
I consider it very handy. For now I support objects, dictionary, tuple and list types of objects.
I guess a test-case and a documentation entry will be needed. But I would first like to get a feedback about the intention of the change.
Thanks
https://bugs.python.org/issue33294