GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-43521: Allow ast.unparse with empty sets and NaN by Kodiologist · Pull Request #24897 · python/cpython

Conversation

@Kodiologist

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue43521

Automerge-Triggered-By: GH:pablogsal

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Kodiologist
Kodiologist requested a review from isidentical as a code owner March 16, 2021 19:41
@the-knights-who-say-ni

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

Copy link
Copy Markdown

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Mar 16, 2021
@bedevere-bot bedevere-bot added the awaiting review label Mar 16, 2021
@Kodiologist Kodiologist mentioned this pull request Mar 16, 2021
Remove the dependency on astor for Pythons ≥ 3.9 hylang/hy#1999
Merged
ericvsmith
ericvsmith reviewed Mar 16, 2021
View reviewed changes
Comment thread Lib/ast.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@isidentical

isidentical commented Mar 16, 2021

Copy link
Copy Markdown
Member

Thanks for the fix @Kodiologist!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericvsmith
ericvsmith reviewed Mar 16, 2021
View reviewed changes
Comment thread Lib/ast.py Outdated

@ericvsmith ericvsmith Mar 16, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nice!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brandtbucher brandtbucher Mar 16, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pablogsal pablogsal Mar 16, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@isidentical isidentical Mar 16, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Kodiologist Kodiologist Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pablogsal pablogsal Mar 16, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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>

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
isidentical reacted with laugh emoji
All reactions
  • 😄 1 reaction
isidentical
isidentical approved these changes Mar 16, 2021
View reviewed changes

@isidentical isidentical left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Mar 16, 2021
ericvsmith
ericvsmith reviewed Mar 16, 2021
View reviewed changes

@ericvsmith ericvsmith left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Kodiologist
Kodiologist force-pushed the unparse-empty-set-and-nan branch 3 times, most recently from ec165a0 to 9cbd740 Compare March 16, 2021 19:59
@pablogsal

pablogsal commented Mar 16, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Kodiologist
bpo-43521: Allow ast.unparse with empty sets and NaN
d811584
@Kodiologist
Kodiologist force-pushed the unparse-empty-set-and-nan branch from 9cbd740 to d811584 Compare March 16, 2021 20:06
@Kodiologist

Kodiologist commented Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

Thanks, done.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brandtbucher

brandtbucher commented Mar 16, 2021

Copy link
Copy Markdown
Member

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.)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal pablogsal added 🤖 automerge labels Mar 16, 2021
@pablogsal

pablogsal commented Mar 16, 2021

Copy link
Copy Markdown
Member

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@isidentical

isidentical commented Mar 18, 2021

Copy link
Copy Markdown
Member

Is automerge stuck?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Mar 18, 2021
@miss-islington
miss-islington merged commit 08ff436 into python:master Mar 18, 2021
@miss-islington

miss-islington commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 18, 2021
@bedevere-bot

bedevere-bot commented Mar 18, 2021

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Mar 18, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 18, 2021
@Kodiologist @miss-islington
bpo-43521: Allow ast.unparse with empty sets and NaN (pythonGH-24897)
4d3cc11 
Automerge-Triggered-By: GH:pablogsal
(cherry picked from commit 08ff436)

Co-authored-by: Kodi Arfer <[email protected]>
@Kodiologist

Kodiologist commented Mar 18, 2021

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington added a commit that referenced this pull request Mar 18, 2021
@miss-islington @Kodiologist
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 <[email protected]>
jab added a commit to jab/cpython that referenced this pull request Mar 20, 2021
@jab
Merge branch 'master' into justin39/aiter-c
97d06e5 
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericvsmith ericvsmith ericvsmith left review comments
@pablogsal pablogsal pablogsal left review comments
@brandtbucher brandtbucher brandtbucher left review comments
@isidentical isidentical isidentical approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@Kodiologist @the-knights-who-say-ni @isidentical @pablogsal @brandtbucher @miss-islington @bedevere-bot @ericvsmith
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.