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 — GH-90699: Remove remaining `_Py_IDENTIFIER` stdlib usage by kumaraditya303 · Pull Request #99067 · python/cpython

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Nov 3, 2022 •
edited by miss-islington
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: Replace _Py_IDENTIFIER() with statically initialized objects. #90699

Automerge-Triggered-By: GH:ericsnowcurrently

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kumaraditya303 added 4 commits November 3, 2022 16:16
@kumaraditya303
port all
6920a94
@kumaraditya303
fix up
a1d2eed
@kumaraditya303
fix rc
ff929eb
@kumaraditya303
fix windows
0788bd6
@bedevere-bot bedevere-bot mentioned this pull request Nov 3, 2022
Replace _Py_IDENTIFIER() with statically initialized objects. #90699
Closed
@bedevere-bot bedevere-bot added the awaiting review label Nov 3, 2022
@kumaraditya303 kumaraditya303 added the skip news label Nov 3, 2022
ericsnowcurrently
ericsnowcurrently requested changes Nov 3, 2022
View reviewed changes
Comment thread Include/internal/pycore_global_strings.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Nov 3, 2022
@bedevere-bot

bedevere-bot commented Nov 3, 2022

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303 added 5 commits November 4, 2022 10:33
@kumaraditya303
Merge branch 'main' of https://github.com/python/cpython into rest-pyid
dcc99db
@kumaraditya303
remove comment
bd64bc7
@kumaraditya303
fix error checking
bf02c24
@kumaraditya303
add comment
350b9c5
@kumaraditya303
clarify comment
9ef5449
@kumaraditya303
kumaraditya303 marked this pull request as ready for review November 4, 2022 11:20
@kumaraditya303
kumaraditya303 requested review from a team, abalkin and pganssle as code owners November 4, 2022 11:20
@kumaraditya303
fix comment
0ccde54
@kumaraditya303
kumaraditya303 requested a review from ericsnowcurrently November 4, 2022 12:33
kumaraditya303 added 3 commits November 4, 2022 17:18
@kumaraditya303
use _py_
729784a
@kumaraditya303
fix comment
f879f00
@kumaraditya303
Merge branch 'main' into rest-pyid
c4c2e96
ericsnowcurrently
ericsnowcurrently approved these changes Nov 4, 2022
View reviewed changes

@ericsnowcurrently ericsnowcurrently 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.

LGTM

(Thanks for the changes.)

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 changes labels Nov 4, 2022
ericsnowcurrently
ericsnowcurrently requested changes Nov 7, 2022
View reviewed changes

@ericsnowcurrently ericsnowcurrently 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.

One small thing and then LGTM.

Also, were you planning on removing _Py_IDENTIFIER() from Programs/_testembed.c? (Presumably we'd drop test_unicode_id_init().) Then again, that should probably be part of the issue/PR that actually eliminates _Py_IDENTIFIER().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/ossaudiodev.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 7, 2022
@bedevere-bot bedevere-bot added the awaiting changes label Nov 7, 2022
@bedevere-bot

bedevere-bot commented Nov 7, 2022

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

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

kumaraditya303 commented Nov 7, 2022

Copy link
Copy Markdown
Contributor Author

Then again, that should probably be part of the issue/PR that actually eliminates _Py_IDENTIFIER().

Yeah, will remove that in PR which removes _Py_IDENTIFIER together, no plans for that though.
ericsnowcurrently 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.
@kumaraditya303
remove #define
044f704
@kumaraditya303
kumaraditya303 requested a review from ericsnowcurrently November 7, 2022 17:33
ericsnowcurrently
ericsnowcurrently approved these changes Nov 7, 2022
View reviewed changes

@ericsnowcurrently ericsnowcurrently 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.

LGTM

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 changes labels Nov 7, 2022
@ericsnowcurrently ericsnowcurrently changed the title GH-90699: Remove remaining _Py_IDENTIFIER usage GH-90699: Remove remaining _Py_IDENTIFIER stdlib usage Nov 7, 2022
@ericsnowcurrently ericsnowcurrently added the 🤖 automerge label Nov 7, 2022
@miss-islington
miss-islington merged commit be0d500 into python:main Nov 7, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 7, 2022
@vstinner

vstinner commented Nov 7, 2022

Copy link
Copy Markdown
Member

Refleak buildbots fail since this commit. Example:
https://buildbot.python.org/all/#/builders/217/builds/878

2 tests failed:
    test_finalization test_gc

Example:

$ make && ./python -m test -R 3:3 test_finalization -m test_legacy_self_cycle 
...
test_finalization leaked [1, 1, 1] references, sum=3
...

cc @pablogsal
ericsnowcurrently 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.
@pablogsal

pablogsal commented Nov 7, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Please @ericsnowcurrently @kumaraditya303 take a look, otherwise we will need to revert if is not fixed in 24h per our buildbot policy.
ericsnowcurrently 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.
@ericsnowcurrently

ericsnowcurrently commented Nov 8, 2022

Copy link
Copy Markdown
Member

If @kumaraditya303 doesn't get to it first, I'll tackle this first thing tomorrow morning (~15 hours from now).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303
kumaraditya303 deleted the rest-pyid branch November 8, 2022 05:23
@kumaraditya303

kumaraditya303 commented Nov 8, 2022

Copy link
Copy Markdown
Contributor Author

#99236 fixes the refleak.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Nov 8, 2022

Copy link
Copy Markdown
Member

The PR got merged, I confirm that it does fix the issue:

$ ./python -m test -R 3:3 test_gc test_finalization
(...)
Tests result: SUCCESS
All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently commented Nov 8, 2022

Copy link
Copy Markdown
Member

Thanks, @kumaraditya303!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes
@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner
@abalkin abalkin Awaiting requested review from abalkin

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

6 participants

@kumaraditya303 @bedevere-bot @vstinner @pablogsal @ericsnowcurrently @miss-islington
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 01402520 — 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.