bpo-15987: Add ast.AST class richcompare methods #14970
Conversation
c81a19c to
4ebc17a
Compare
Lib/test/test_ast.py
Outdated
| self.assertEqual(ast.Str("ABCD"), ast.Str("ABCD")) | ||
| self.assertEqual(ast.Str("中文字"), ast.Str("中文字")) | ||
|
|
||
| self.assertNotEqual(ast.Num(10), ast.Num(20)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@asottile Current implementation returns False, so that's all good
There was a problem hiding this comment.
awesome, just wanted to make sure that's captured in the tests
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
This issue has been resolved.
There was a problem hiding this comment.
From a quick glance, I can see multiple issues with this PR:
- There are multiple reference leaks involving
PyObject_GetAttr()andPyObject_GetAttrString(). - C API functions like
PyObject_GetAttrString()andPySequence_Size()are not
checked for failure. - In
ast_richcompare(),lenandishould be of typePy_ssize_t, notint. - 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 :
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
|
|
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 |
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>
f31c87b to
15c5f1f
Compare
|
🤖 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? |
|
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 |
Recreation of #1368, updated to current master.
https://bugs.python.org/issue15987