Skip to content

Conversation

@kumaraditya303
Copy link
Contributor

@kumaraditya303kumaraditya303 commented Jan 20, 2022

@kumaraditya303
Copy link
ContributorAuthor

It is an implementation detail so skip news.

@gvanrossum
Copy link
Member

It is an implementation detail so skip news.

I don't think that's how we usually do this. We skip news for things like fixing typos in comments, but almost anything else gets a new entry. Maybe you're confused with "What's New in 3.11"? That definitely only should get the major visible items.

Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

LG except I would like to see a one-line news item.

Did you check how much space this saves? (Probably nut that much since ints are small.)

@kumaraditya303
Copy link
ContributorAuthor

It is an implementation detail so skip news.

I don't think that's how we usually do this. We skip news for things like fixing typos in comments, but almost anything else gets a new entry. Maybe you're confused with "What's New in 3.11"? That definitely only should get the major visible items.

Ok, I'll add a news fragment. Should I create a bpo for this now ?

@gvanrossum
Copy link
Member

Should I create a bpo for this now ?

Yeah.

@kumaraditya303kumaraditya303 changed the title deepfreeze use preallocated small intsbpo-46443: deepfreeze use preallocated small intsJan 20, 2022
@gvanrossum
Copy link
Member

I recommend that you put a little more effort in writing the initial comment for the bpo issues you create. E.g. not just what to do, but why it's worth doing and what impact you expect.

Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks! This was indeed super-simple. :-)

@kumaraditya303
Copy link
ContributorAuthor

kumaraditya303 commented Jan 20, 2022

I would also do this for empty bytes object as that also has a global singleton. (DONE)

@gvanrossum
Copy link
Member

I would also do this for empty bytes object as that also has a global singleton but separately.

Good idea. Maybe you can just add that to this PR, it's small enough.

@kumaraditya303
Copy link
ContributorAuthor

It is an implementation detail so skip news.

I don't think that's how we usually do this. We skip news for things like fixing typos in comments, but almost anything else gets a new entry. Maybe you're confused with "What's New in 3.11"? That definitely only should get the major visible items.

I was confused by this PR as it skipped news #30092

Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

Great! Now just waiting for CI to finish...

@gvanrossum
Copy link
Member

I was confused by this PR as it skipped news #30092

Different core devs have different standards...

@gvanrossumgvanrossum merged commit 194ecc6 into python:mainJan 20, 2022
@gvanrossum
Copy link
Member

I'm the bot. :-)

@kumaraditya303kumaraditya303 deleted the longobj branch January 20, 2022 06:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@kumaraditya303@gvanrossum@bedevere-bot