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

flavianh · 2019-07-26T20:49:23Z

Recreation of #1368, updated to current master.

https://bugs.python.org/issue15987

asottile · 2019-07-29T14:37:39Z

Lib/test/test_ast.py

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

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

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

@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?

This issue has been resolved.

ZackerySpytz

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.

isidentical · 2020-02-09T10:41:05Z

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 · 2020-03-25T23:06:36Z

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 · 2020-03-27T12:57:10Z

Sorry about the lack of activity, I’ve been fairly busy and covid issues do not help either. I’ll try to have a look this weekend. Le jeu. 26 mars 2020 à 00:06, Batuhan Taşkaya <notifications@github.com> a écrit :

Second ping (first one was after 7 months, this one is after 9 months in any kind of activity) @flavianh <https://github.com/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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14970 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJF2USGCRJJIZ3APX2R3LTRJKFA5ANCNFSM4IHHCPWQ> .

-- F. Hautbois

isidentical · 2020-03-27T13:55:39Z

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 · 2020-03-27T18:24:25Z

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

Co-authored-by: Louie Lu <me@louie.lu> Co-authored-by: Batuhan Taşkaya <batuhanosmantaskaya@gmail.com>

Co-authored-by: Batuhan Taşkaya <batuhanosmantaskaya@gmail.com>

bedevere-bot · 2020-03-28T02:39:45Z

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

isidentical · 2020-03-28T03:22:17Z

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

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

serhiy-storchaka · 2020-03-28T14:48:59Z

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 · 2022-07-17T21:46:09Z

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

the-knights-who-say-ni added the CLA signed label Jul 26, 2019

bedevere-bot added the awaiting review label Jul 26, 2019

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

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

Closed

asottile reviewed Jul 29, 2019

View reviewed changes

asottile approved these changes Jul 29, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jul 29, 2019

ZackerySpytz suggested changes Jul 30, 2019

View reviewed changes

pablogsal self-assigned this Dec 1, 2019

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

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

Implement hashing

9652964

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

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

use tokenize.open instead of open

552232c

flavianh commented Mar 28, 2020

View reviewed changes

isidentical requested a review from serhiy-storchaka March 28, 2020 11:22

ezio-melotti removed the CLA signed label Jul 13, 2022

hauntsaninja closed this Jul 17, 2022

Julian mannequin mentioned this pull request Apr 10, 2022

Provide a way to compare AST nodes for equality recursively #60191

Open

Uh oh!

Comments

Conversation

flavianh commented Jul 26, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

flavianh Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

asottile Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

flavianh Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isidentical Mar 28, 2020

Choose a reason for hiding this comment

Uh oh!

ZackerySpytz left a comment

Choose a reason for hiding this comment

Uh oh!

isidentical commented Feb 9, 2020

Uh oh!

isidentical commented Mar 25, 2020

Uh oh!

flavianh commented Mar 27, 2020 via email

Uh oh!

isidentical commented Mar 27, 2020

Uh oh!

flavianh commented Mar 27, 2020

Uh oh!

bedevere-bot commented Mar 28, 2020

Uh oh!

isidentical commented Mar 28, 2020

Uh oh!

flavianh left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Mar 28, 2020

Uh oh!

hauntsaninja commented Jul 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

flavianh commented Jul 26, 2019 •

edited by bedevere-bot

Loading

flavianh Jul 29, 2019 •

edited

Loading