Skip to content

gh-123504: Fix regression in _tkinter initializer - #123662

Merged
vstinner merged 6 commits into
python:mainfrom
ZeroIntensity:tkinter-regression
Sep 4, 2024
Merged

gh-123504: Fix regression in _tkinter initializer #123662
vstinner merged 6 commits into
python:mainfrom
ZeroIntensity:tkinter-regression

Conversation

@ZeroIntensity

@ZeroIntensityZeroIntensity commented Sep 4, 2024

Copy link
Copy Markdown
Member

@ZeroIntensity

ZeroIntensity commented Sep 4, 2024

Copy link
Copy Markdown
MemberAuthor

(@encukou, this needs skip news.)

Comment threadModules/_tkinter.c Outdated
@picnixz

picnixz commented Sep 4, 2024

Copy link
Copy Markdown
Member

You can @ either me or @Eclips4 for skip news requests since we are triagers, or we'll just see whether this is needed or not when we'll look at the PR.

@ZeroIntensity

Copy link
Copy Markdown
MemberAuthor

You can @ either me or @Eclips4 for skip news requests since we are triagers

I would, but Petr specifically requested that I ping him for label changes (he's "mentoring" me for triage membership).

@ZeroIntensity

Copy link
Copy Markdown
MemberAuthor

Does that look alright?

@picnixz
picnixz self-requested a review September 4, 2024 11:00

@picnixzpicnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good! If I were to nitpick I would have (in the previous PR that I wasn't able to comment since it was merged before):

  • Py_UNUSED(module) in the module_traverse signature
  • Py_UNUSED(mod) in the module_clear signature
  • (void)module_clear((PyObject *)mod); in module_free
  • Renamed the functions to _tkintermodule_{clear,traverse,free}.

You could do those cosmetic changes if you want but otherwise it's fine.

@ZeroIntensity

Copy link
Copy Markdown
MemberAuthor

I'm going to leave the names as is, we can change them if we decide to migrate tkinter over to PEP 489

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@vstinner
vstinner merged commit 2daed5f into python:mainSep 4, 2024
@ZeroIntensity
ZeroIntensity deleted the tkinter-regression branch September 4, 2024 19:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ZeroIntensity@picnixz@vstinner@serhiy-storchaka