X Tutup
Skip to content

bpo-35728: Add root parameter to tkinter.font.nametofont()#23885

Merged
serhiy-storchaka merged 4 commits intopython:masterfrom
desmondcheongzx:bpo-35728-add-root-parameter-to-tkinter-font-nametofont
Dec 25, 2020
Merged

bpo-35728: Add root parameter to tkinter.font.nametofont()#23885
serhiy-storchaka merged 4 commits intopython:masterfrom
desmondcheongzx:bpo-35728-add-root-parameter-to-tkinter-font-nametofont

Conversation

@desmondcheongzx
Copy link
Copy Markdown
Contributor

@desmondcheongzx desmondcheongzx commented Dec 22, 2020 •
edited by bedevere-bot
Loading

Add a root parameter to tkinter.font.nametofont() so that the function can be called even when default root is disabled.
https://bugs.python.org/issue35728

https://bugs.python.org/issue35728

serhiy-storchaka
serhiy-storchaka reviewed Dec 22, 2020
View reviewed changes
serhiy-storchaka
serhiy-storchaka approved these changes Dec 22, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

terryjreedy
terryjreedy approved these changes Dec 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

The function is not terribly useful, but now is consistent with other functions. Tests look good. Thanks..

Fidget-Spinner
Fidget-Spinner reviewed Dec 25, 2020
View reviewed changes
Doc/library/tkinter.font.rst Outdated
Return a :class:`Font` representation of a tk named font. No newline at end of file
Return a :class:`Font` representation of a tk named font.

.. versionchanged:: 3.10
Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner Dec 25, 2020

Choose a reason for hiding this comment

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

Seems to be missing a space here, indentation in rST is 3 spaces. On some versions of sphinx this causes visual differences.

Suggested change
.. versionchanged:: 3.10
.. versionchanged:: 3.10
Added the *root* keyword argument.

Other than that, LGTM :).

Copy link
Copy Markdown
Contributor Author

@desmondcheongzx desmondcheongzx Dec 25, 2020

Choose a reason for hiding this comment

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

Thanks for catching that! I've updated the documentation accordingly

@terryjreedy
Copy link
Copy Markdown
Member

terryjreedy commented Dec 25, 2020 •
edited by bedevere-bot
Loading

Both Travis and Pipelines failed the doc check of the news item, which I believe is unchanged from the previous run that passed. I don't understand the complaint or know if the test was changed. I restarted Travis and now it passes. Maybe the transient failure was in github. I don't see any way to rerun Pipelines, but since it is the same test, and it passes before, I would consider ignoring it.

python3 tools/rstlint.py ../Misc/NEWS.d/next/
[2] ../Misc/NEWS.d/next/Library/2020-12-23-19-43-06.[bpo-42727](https://bugs.python.org/issue42727).WH3ODh.rst:1: default role used
[2] ../Misc/NEWS.d/next/Library/2020-12-23-19-43-06.[bpo-42727](https://bugs.python.org/issue42727).WH3ODh.rst:2: default role used
2 problems with severity 2 found.
Makefile:204: recipe for target 'check' failed

@Fidget-Spinner
Copy link
Copy Markdown
Member

Fidget-Spinner commented Dec 25, 2020 •
edited
Loading

Both Travis and Pipelines failed the doc check of the news item, which I believe is unchanged from the previous run that passed. I don't understand the complaint or know if the test was changed. I restarted Travis and now it passes. Maybe the transient failure was in github. I don't see any way to rerun Pipelines, but since it is the same test, and it passes before, I would consider ignoring it.

The error means that another news item in another bpo used single backticks rather than double backticks for styling code. The news item was recently fixed in 8badade. Pulling in the latest changes from master into this PR will fix it.

@terryjreedy
Copy link
Copy Markdown
Member

terryjreedy commented Dec 25, 2020 •
edited
Loading

I just saw the checkin of #23932 which changed the news entry for #23917, which at least once had the exact same failure, by doubling the backticks. Perhaps that is needed here also.
@gpshead Please comment.

@terryjreedy
Copy link
Copy Markdown
Member

terryjreedy commented Dec 25, 2020

Messages crossed. @desmondcheongzx please do the update with fetch and merge (or simple pull) rather than with 'force' argument, as some mistakenly do. It is too easy to mess up PR with the latter.

@desmondcheongzx
Copy link
Copy Markdown
Contributor Author

desmondcheongzx commented Dec 25, 2020

Got it, I've made the appropriate update.

@serhiy-storchaka serhiy-storchaka merged commit 36a779e into python:master Dec 25, 2020
@serhiy-storchaka
Copy link
Copy Markdown
Member

serhiy-storchaka commented Dec 25, 2020

Thank you for your contribution @desmondcheongzx!

@desmondcheongzx
Copy link
Copy Markdown
Contributor Author

desmondcheongzx commented Dec 25, 2020

Thank you for the reviews!

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@desmondcheongzx @adorilson
bpo-35728: Add root parameter to tkinter.font.nametofont() (pythonGH-…
a3ceab0 
…23885)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@terryjreedy terryjreedy terryjreedy approved these changes

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@desmondcheongzx @terryjreedy @Fidget-Spinner @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot
X Tutup