bpo-43521: Allow ast.unparse with empty sets and NaN #24897

Kodiologist · 2021-03-16T19:41:10Z

https://bugs.python.org/issue43521

Automerge-Triggered-By: GH:pablogsal

the-knights-who-say-ni · 2021-03-16T19:41:13Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@Kodiologist

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Lib/ast.py

isidentical · 2021-03-16T19:49:27Z

Thanks for the fix @Kodiologist!

ericvsmith · 2021-03-16T19:49:31Z

Lib/ast.py

+        else:
+            # `{}` would be interpreted as a dictionary literal, and
+            # `set` might not be shadowed. Thus:
+            self.write('{1}.__class__()')


Perhaps {*()} would be a bit more concise?

I agree, the {1} can be a bit confusing.

just for the reference, the proposed was is how astor does it. Though I think {*()} is also a good alternative

Yep, I'm the one who wrote that astor patch. I didn't realize {*()} was a possibility.

< joking>

What about this nuclear option?

>>> ctypes.pythonapi.PySet_New.argtypes = ctypes.py_object, >>> ctypes.pythonapi.PySet_New.restype = ctypes.py_object >>> ctypes.pythonapi.PySet_New([]).__class__ <class 'set'>

< /joking>

isidentical

Overall it looks fine to me. One little nit is that, can you please adjust your style (like end parens on different lines) to match with the other parts of the code?

E.g;

self.write(
    repr(value)
    .replace("inf", _INFSTR)
    .replace("nan", "({}-{})".format(_INFSTR, _INFSTR))
)

(same goes for the tests)

ericvsmith

This looks good to me, but I'm not an expert in this module. @pablogsal should probably take a look at it.

pablogsal · 2021-03-16T20:01:43Z

Looks fantastic. @Kodiologist could you also add a NEWS entry using blurb or https://blurb-it.herokuapp.com/ ?

Kodiologist · 2021-03-16T20:07:08Z

Thanks, done.

brandtbucher · 2021-03-16T20:12:06Z

Also, no need to force-push. The commits will be squashed anyways when your patch is merged.

(I find it's better to keep a full commit history here, since it gives context to the discussions if we need to come back to this later.)

pablogsal · 2021-03-16T20:25:45Z

I plan to backport this because I consider it close enough to the "bug" territory (although is within the initial design constraints). If someone disagrees, please, express yourself and we can remove the label.

isidentical · 2021-03-18T17:33:10Z

Is automerge stuck?

miss-islington · 2021-03-18T17:36:09Z

Thanks @Kodiologist for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

bedevere-bot · 2021-03-18T17:36:48Z

GH-24921 is a backport of this pull request to the 3.9 branch.

Automerge-Triggered-By: GH:pablogsal (cherry picked from commit 08ff436) Co-authored-by: Kodi Arfer <Kodiologist@users.noreply.github.com>

Kodiologist · 2021-03-18T17:36:51Z

Ah, I had to use https://check-python-cla.herokuapp.com to get the bots to notice that I'd signed the CLA.

Automerge-Triggered-By: GH:pablogsal (cherry picked from commit 08ff436) Co-authored-by: Kodi Arfer <Kodiologist@users.noreply.github.com>

* master: (129 commits) bpo-43452: Micro-optimizations to PyType_Lookup (pythonGH-24804) bpo-43517: Fix false positive in detection of circular imports (python#24895) bpo-43494: Make some minor changes to lnotab notes (pythonGH-24861) Mention that code.co_lnotab is deprecated in what's new for 3.10. (python#24902) bpo-43244: Remove symtable.h header file (pythonGH-24910) bpo-43466: Add --with-openssl-rpath configure option (pythonGH-24820) Fix a typo in c-analyzer (pythonGH-24468) bpo-41561: Add workaround for Ubuntu's custom security level (pythonGH-24915) bpo-43521: Allow ast.unparse with empty sets and NaN (pythonGH-24897) bpo-43244: Remove the PyAST_Validate() function (pythonGH-24911) bpo-43541: Fix PyEval_EvalCodeEx() regression (pythonGH-24918) bpo-43244: Fix test_peg_generators on Windows (pythonGH-24913) bpo-39342: Expose X509_V_FLAG_ALLOW_PROXY_CERTS in ssl module (pythonGH-18011) bpo-43244: Fix test_peg_generator for PyAST_Validate() (pythonGH-24912) bpo-42128: Add 'missing :' syntax error message to match statements (pythonGH-24733) bpo-43244: Add pycore_ast.h header file (pythonGH-24908) bpo-43244: Rename pycore_ast.h to pycore_ast_state.h (pythonGH-24907) Remove unnecessary imports in the grammar parser (pythonGH-24904) bpo-35883: Py_DecodeLocale() escapes invalid Unicode characters (pythonGH-24843) Add PEP 626 to what's new in 3.10. (python#24892) ...

Kodiologist requested a review from isidentical as a code owner March 16, 2021 19:41

the-knights-who-say-ni added the CLA not signed label Mar 16, 2021

bedevere-bot added the awaiting review label Mar 16, 2021

Kodiologist mentioned this pull request Mar 16, 2021

Remove the dependency on astor for Pythons ≥ 3.9 hylang/hy#1999

Merged

ericvsmith reviewed Mar 16, 2021

View reviewed changes

Lib/ast.py Outdated Show resolved Hide resolved

ericvsmith reviewed Mar 16, 2021

View reviewed changes

isidentical approved these changes Mar 16, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Mar 16, 2021

ericvsmith reviewed Mar 16, 2021

View reviewed changes

Kodiologist force-pushed the unparse-empty-set-and-nan branch 3 times, most recently from ec165a0 to 9cbd740 Compare March 16, 2021 19:59

bpo-43521: Allow ast.unparse with empty sets and NaN

d811584

Kodiologist force-pushed the unparse-empty-set-and-nan branch from 9cbd740 to d811584 Compare March 16, 2021 20:06

pablogsal added 🤖 automerge needs backport to 3.9 only security fixes labels Mar 16, 2021

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Mar 18, 2021

miss-islington merged commit 08ff436 into python:master Mar 18, 2021

bedevere-bot removed the awaiting merge label Mar 18, 2021

bedevere-bot removed the needs backport to 3.9 only security fixes label Mar 18, 2021

miss-islington added a commit that referenced this pull request Mar 18, 2021

bpo-43521: Allow ast.unparse with empty sets and NaN (GH-24897)

e8e3419

Automerge-Triggered-By: GH:pablogsal (cherry picked from commit 08ff436) Co-authored-by: Kodi Arfer <Kodiologist@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-43521: Allow ast.unparse with empty sets and NaN #24897

bpo-43521: Allow ast.unparse with empty sets and NaN #24897

Kodiologist commented Mar 16, 2021 •

edited by miss-islington

Loading

the-knights-who-say-ni commented Mar 16, 2021

isidentical commented Mar 16, 2021

ericvsmith Mar 16, 2021

brandtbucher Mar 16, 2021

pablogsal Mar 16, 2021

isidentical Mar 16, 2021

Kodiologist Mar 16, 2021

pablogsal Mar 16, 2021 •

edited

Loading

isidentical left a comment

ericvsmith left a comment

pablogsal commented Mar 16, 2021

Kodiologist commented Mar 16, 2021

brandtbucher commented Mar 16, 2021

pablogsal commented Mar 16, 2021

isidentical commented Mar 18, 2021

miss-islington commented Mar 18, 2021

bedevere-bot commented Mar 18, 2021

Kodiologist commented Mar 18, 2021

bpo-43521: Allow ast.unparse with empty sets and NaN #24897

bpo-43521: Allow ast.unparse with empty sets and NaN #24897

Conversation

Kodiologist commented Mar 16, 2021 • edited by miss-islington Loading

the-knights-who-say-ni commented Mar 16, 2021

CLA Missing

isidentical commented Mar 16, 2021

ericvsmith Mar 16, 2021

Choose a reason for hiding this comment

brandtbucher Mar 16, 2021

Choose a reason for hiding this comment

pablogsal Mar 16, 2021

Choose a reason for hiding this comment

isidentical Mar 16, 2021

Choose a reason for hiding this comment

Kodiologist Mar 16, 2021

Choose a reason for hiding this comment

pablogsal Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

isidentical left a comment

Choose a reason for hiding this comment

ericvsmith left a comment

Choose a reason for hiding this comment

pablogsal commented Mar 16, 2021

Kodiologist commented Mar 16, 2021

brandtbucher commented Mar 16, 2021

pablogsal commented Mar 16, 2021

isidentical commented Mar 18, 2021

miss-islington commented Mar 18, 2021

bedevere-bot commented Mar 18, 2021

Kodiologist commented Mar 18, 2021

Kodiologist commented Mar 16, 2021 •

edited by miss-islington

Loading

pablogsal Mar 16, 2021 •

edited

Loading