Skip to content

Clarify news entry for --trim #58992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: release-1.12
Choose a base branch
from

Conversation

ajinkya-k
Copy link
Contributor

@ajinkya-k ajinkya-k commented Jul 14, 2025

The news entry for --trim in release 1.12 uses a double negative which is a little confusing. This PR fixes that

EDIT: added description

@ajinkya-k ajinkya-k force-pushed the ahk/news-patch-1.12 branch from b538d06 to 6800d81 Compare July 14, 2025 03:02
@ajinkya-k ajinkya-k force-pushed the ahk/news-patch-1.12 branch from 6800d81 to 1de2eb9 Compare July 14, 2025 03:03
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

The change is incorrect. The trim feature works as stated, not as proposed

@ajinkya-k
Copy link
Contributor Author

@Keno in that case maybe it shoould be rephrased as "only keeps code that is provably reachable from the entry point"?

@LilithHafner
Copy link
Member

Removing the double negative is is fine as long as it doesn't change the semantics. Notably "proven" and "provable" are different concepts.

@ajinkya-k
Copy link
Contributor Author

ajinkya-k commented Jul 14, 2025

Removing the double negative is is fine as long as it doesn't change the semantics. Notably "proven" and "provable" are different concepts.

Thanks! I changed the language based on the language used in the trim PR #55047

LilithHafner
LilithHafner previously approved these changes Jul 14, 2025
Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

I think this is now correct but defer to @Keno for a final review.

@LilithHafner LilithHafner requested a review from Keno July 14, 2025 15:46
@adienes
Copy link
Member

adienes commented Jul 14, 2025

I will note that the word "statically" was explicitly discouraged here #56460 (comment)

@LilithHafner LilithHafner dismissed their stale review July 14, 2025 15:54

updated context

@LilithHafner LilithHafner changed the title fix news entry for --trim Clarify news entry for --trim again Jul 14, 2025
@ajinkya-k ajinkya-k force-pushed the ahk/news-patch-1.12 branch from d9634e4 to c71f1cf Compare July 14, 2025 16:29
@ajinkya-k
Copy link
Contributor Author

I will note that the word "statically" was explicitly discouraged here #56460 (comment)

fixed, thanks!

@ajinkya-k
Copy link
Contributor Author

@ericphanson Thanks, fixed

@ajinkya-k ajinkya-k changed the title Clarify news entry for --trim again Clarify news entry for --trim Jul 14, 2025
@JeffBezanson
Copy link
Member

This wording is fine but these are very annoying to backport since the change has to be moved to a separate file.

Sign up for free to 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.

6 participants