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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

This issue has been resolved.

Choose a reason for hiding this comment

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

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.

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)

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.

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 :

-- F. Hautbois

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.

@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

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>

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

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

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?

Choose a reason for hiding this comment

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

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

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.

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