FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-15987: Add ast.AST class richcompare methods by flavianh · Pull Request #14970 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-15987: Add ast.AST class richcompare methods - #14970

Closed
flavianh wants to merge 7 commits into
python:mainfrom
flavianh:add-ast-class-rich-compare-methods
ConversationCommits (7)ChecksFiles changed
Closed

bpo-15987: Add ast.AST class richcompare methods #14970
flavianh wants to merge 7 commits into
python:mainfrom
flavianh:add-ast-class-rich-compare-methods

Conversation

flavianh commented Jul 26, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown

Recreation of #1368, updated to current master.

https://bugs.python.org/issue15987

flavianh force-pushed the add-ast-class-rich-compare-methods branch from c81a19c to 4ebc17a Compare July 26, 2019 21:35
flavianh mentioned this pull request Jul 27, 2019
Closed
asottile reviewed Jul 29, 2019
View reviewed changes
Comment thread Lib/test/test_ast.py Outdated

asottile Jul 29, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can you add a testcase for ast.Num(10) and ast.Num(10.0) -- I suspect the current implementation will return True but perhaps it should not

The same for ast.Num(1) and ast.NamedConstant(True) is probably also a good test

flavianh Jul 29, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@asottile Current implementation returns False, so that's all good

asottile Jul 29, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

awesome, just wanted to make sure that's captured in the tests

flavianh Jul 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@asottile wait hold on, I added it in the tests and it passed locally when I tried it in my re-compiled 3.9 version, but on the CI it does not. I guess it will need some fixing after all. Good catch! Any idea how to fix this?

isidentical Mar 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This issue has been resolved.

ZackerySpytz suggested changes Jul 30, 2019
View reviewed changes

ZackerySpytz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

From a quick glance, I can see multiple issues with this PR:

  • There are multiple reference leaks involving PyObject_GetAttr() and PyObject_GetAttrString().
  • C API functions like PyObject_GetAttrString() and PySequence_Size() are not
    checked for failure.
  • In ast_richcompare(), len and i should be of type Py_ssize_t, not int.
  • Some code does not follow PEP 7 (braces are required for all ifs) https://www.python.org/dev/peps/pep-0007/#code-lay-out.

pablogsal self-assigned this Dec 1, 2019
pablogsal self-requested a review December 1, 2019 13:28

isidentical commented Feb 9, 2020

Copy link
Copy Markdown
Member

What's the status of this PR? IMHO it would be so cool to see this in 3.9, before the alpha period ends (~3 months)

isidentical commented Mar 25, 2020

Copy link
Copy Markdown
Member

Second ping (first one was after 7 months, this one is after 9 months in any kind of activity) @flavianh are you still interested in this PR? If you are it would be really cool to apply reviews from code suggestions and sync with master. I'd really like to see this before the alpha cut.

flavianh commented Mar 27, 2020 via email

Copy link
Copy Markdown
Author

isidentical commented Mar 27, 2020

Copy link
Copy Markdown
Member

No need for sorry, thanks for looking into this issue. If you can't find the time, I can push these suggestions and rebase your PR to master if you could just give me access to your fork.

flavianh commented Mar 27, 2020

Copy link
Copy Markdown
Author

@isidentical That would be very helpful yes :). I've added you to my fork

Flavian Hautbois and others added 5 commits March 27, 2020 22:32
bpo-15987: Add ast.AST class richcompare methods
b9cb098 
Co-authored-by: Louie Lu <me@louie.lu>
Co-authored-by: Batuhan Taşkaya <batuhanosmantaskaya@gmail.com>
Credit Louie Lu for original code
c82f468 
Co-authored-by: Batuhan Taşkaya <batuhanosmantaskaya@gmail.com>
Check int vs float and bool vs int inequality
4d14244 
Co-authored-by: Batuhan Taşkaya <batuhanosmantaskaya@gmail.com>
improve tests
d99ff0a
Refactor rich_compare method with resolving refleaks
15c5f1f
isidentical force-pushed the add-ast-class-rich-compare-methods branch from f31c87b to 15c5f1f Compare March 28, 2020 01:19
isidentical added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 28, 2020

bedevere-bot commented Mar 28, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @isidentical for commit 9652964 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 28, 2020

isidentical commented Mar 28, 2020

Copy link
Copy Markdown
Member

PR rebased to master, tests updated, the main richcompare code refactored, cleared from all its refleaks and exception handling is improved. @flavianh can you review the changes?

flavianh commented Mar 28, 2020
View reviewed changes

flavianh left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but I can't approve it since I'm the owner of the PR. Thanks for taking over ;)

serhiy-storchaka commented Mar 28, 2020

Copy link
Copy Markdown
Member

There are some issues with the code, but I do not point on them because I am not sure that implementing the rich comparison of AST is a good idea. See my objections on the bug tracker.

hauntsaninja commented Jul 17, 2022

Copy link
Copy Markdown
Contributor

At least two core devs feel a dedicated function is better, so closing this. Superseded by #19211

Julian mannequin mentioned this pull request Apr 10, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

isidentical isidentical left review comments
pablogsal Awaiting requested review from pablogsal pablogsal is a code owner
serhiy-storchaka Awaiting requested review from serhiy-storchaka
+2 more reviewers
asottile asottile approved these changes
ZackerySpytz ZackerySpytz requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

pablogsal

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL