Movatterモバイル変換

bedevere-bot added the awaiting review label

Kodiologist mentioned this pull request

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

Merged

ericvsmith reviewed

Lib/ast.py OutdatedShow resolvedHide resolved

Copy link

Member

isidentical commentedMar 16, 2021

Thanks for the fix@Kodiologist!

ericvsmith reviewed

Lib/ast.py Outdated

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

Copy link

Member

Choose a reason for hiding this comment

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

Nice!

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Member

pablogsalMar 16, 2021•
edited
Loading

Choose a reason for hiding this comment

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

< 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 approved these changes

Copy link

Member

isidentical left a comment

Choose a reason for hiding this comment

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

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)

bedevere-bot added awaiting merge and removed awaiting review labels

ericvsmith reviewed

Copy link

Member

ericvsmith left a comment

Choose a reason for hiding this comment

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

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

Kodiologist force-pushed theunparse-empty-set-and-nan branch 3 times, most recently fromec165a0 to9cbd740Compare

March 16, 2021 19:59

Copy link

Member

pablogsal commentedMar 16, 2021

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

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

d811584

Kodiologist force-pushed theunparse-empty-set-and-nan branch from9cbd740 tod811584Compare

March 16, 2021 20:06

Copy link

ContributorAuthor

Kodiologist commentedMar 16, 2021

Thanks, done.

Copy link

Member

brandtbucher commentedMar 16, 2021

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 added 🤖 automerge needs backport to 3.9only security fixes labels

Copy link

Member

pablogsal commentedMar 16, 2021

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.

Copy link

Member

isidentical commentedMar 18, 2021

Is automerge stuck?

the-knights-who-say-ni added CLA signed and removed CLA not signed labels

miss-islington merged commit08ff436 intopython:master

Copy link

Contributor

miss-islington commentedMar 18, 2021

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

bedevere-bot removed the awaiting merge label

Copy link

bedevere-bot commentedMar 18, 2021

GH-24921 is a backport of this pull request to the3.9 branch.

bedevere-bot removed the needs backport to 3.9only security fixes label

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

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

4d3cc11

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

Copy link

ContributorAuthor

Kodiologist commentedMar 18, 2021

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

miss-islington added a commit that referenced this pull request