Skip to content

gh-131740: minor readability fix in PyUnstable_GC_VisitObjects#131786

Merged
corona10 merged 2 commits intopython:mainfrom
martindemello:gc-visit
Mar 28, 2025
Merged

gh-131740: minor readability fix in PyUnstable_GC_VisitObjects#131786
corona10 merged 2 commits intopython:mainfrom
martindemello:gc-visit

Conversation

@martindemello
Copy link
Copy Markdown
Contributor

@martindemello martindemello commented Mar 27, 2025 •
edited by bedevere-app bot
Loading

Replaces if (visit_generation()) with if (visit_generation() < 0), since we are checking for the failure case, and it's confusing to have that be implicitly true.

Also fixes a misspelt variable name.

@martindemello
Minor readability fix in PyUnstable_GC_VisitObjects
d511f2a 
Replaces `if (visit_generation())` with `if (visit_generation() < 0)`,
since we are checking for the failure case, and it's confusing to have
that be implicitly `true`.

Also fixes a misspelt variable name.
@ghost
Copy link
Copy Markdown

ghost commented Mar 27, 2025 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@picnixz picnixz changed the title Minor readability fix in PyUnstable_GC_VisitObjects gh-131740: minor readability fix in PyUnstable_GC_VisitObjects Mar 28, 2025
@picnixz
Copy link
Copy Markdown
Member

picnixz commented Mar 28, 2025 •
edited
Loading

I'm willing to accept that change (at least the typo fix) but let's ask @corona10 about this. The pattern for error checks is common.

@bedevere-app bedevere-app bot mentioned this pull request Mar 28, 2025
Closed
@corona10 corona10 self-requested a review March 28, 2025 23:50
corona10
corona10 approved these changes Mar 28, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm.

@corona10 corona10 added needs backport to 3.13 bugs and security fixes and removed needs backport to 3.13 bugs and security fixes labels Mar 28, 2025
@corona10
Copy link
Copy Markdown
Member

corona10 commented Mar 28, 2025

Let's backport to 3.13, too; it's not a bug, but make it as consistent as possible. For 3.12, it's not worth doing that since we need to make a manual backport anyway due to different directory hiarchy.

@corona10 corona10 merged commit 9c1e85f into python:main Mar 28, 2025
53 checks passed
@miss-islington-app
Copy link
Copy Markdown

miss-islington-app bot commented Mar 28, 2025

Thanks @martindemello for the PR, and @corona10 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link
Copy Markdown

miss-islington-app bot commented Mar 28, 2025

Sorry, @martindemello and @corona10, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 9c1e85fd64cf6a85f7c96390d7b2dfad03a76944 3.13

@corona10 corona10 removed the needs backport to 3.13 bugs and security fixes label Mar 28, 2025
@corona10
Copy link
Copy Markdown
Member

corona10 commented Mar 29, 2025 •
edited
Loading

Hmm let's skip backport to 3.13 also. It doesn't look like worth do it since we need to do manual backport.

@martindemello martindemello deleted the gc-visit branch March 31, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 approved these changes

Assignees

@corona10 corona10

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@martindemello @picnixz @corona10 @ZeroIntensity