bpo-29446: Improve tkinter 'import *' situation by flavianh · Pull Request #14864 · python/cpython

flavianh · 2019-07-19T15:59:22Z

A pull request for this issue.

@ericvsmith recommended using __all__ = [name for name, obj in globals().items() if not name.startswith('_') and not isinstance(obj, types.ModuleType) and name not in {'wantobjects'}].

Also added all in submodules of tkinter.

Side note: Interestingly, idlelib/editor.py was using sys and re from tkinter instead of importing them. There may be more modules that use "from tkinter import *"

https://bugs.python.org/issue29446

serhiy-storchaka · 2019-07-19T17:44:11Z

_default_root should never be imported, neither implicitly, nor implicitly.

_cnfmerge should not be imported implicitly since it is an internal function.

_default_root is already imported in tkinter.simpledialog and _cnfmerge is used in dialog.py

For _cnfmerge the change is straightforward: I just have to omit it in __all__ and the import in dialog.py will work just fine

For _default_root I don't know how to remove it from simpledialog.py without affecting the behaviour of _QueryDialog. I can't leave parent as None if no parent is passed because then the line if parent.winfo_viewable(): will fail.

Same in font.py.

I updated the PR to not export _cnfmerge and _default_root in __all__, but could not prevent _default_root from being used elsewhere.

No, it is not imported, but is accessed as an attribute of tkinter.

The difference is that its value is changed after importing. When you import the submodule, it is None. After creating a root widget it keeps a reference to it, which can be used as a default parent in _QueryDialog.

Thanks for the clarification!

serhiy-storchaka · 2019-07-19T17:57:04Z

This is an unrelated change.

True, I dismissed those changes. Can I open a new PR to add this change?

serhiy-storchaka · 2019-07-19T17:59:19Z

Could not this be written in one line?

I was unsure about the code style here. What's the standard?
I reworked the imports though, so there might not be any problem any more

terryjreedy

Flavian, thank you for making a start on this; As for changes, I pretty much agree with Serhiy.
1 Use Eric's suggestion for things defined in .__init__ and exclude submodules.
2. Make lists horizontal, with multiple items per line (this is pretty standard).
3. Put PEP 8 style changes in another PR, or even another issue; but only after asking Serhiy if he wants them, or at least would merge them.
4. I never wrote about submodules, but __all__ for each, except Tix (leave alone), would be nice.

bedevere-bot · 2019-07-19T21:37:53Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

terryjreedy · 2019-07-19T21:44:13Z

With respect to the list in your initial comment: The 'Var's are absolutely essential. Without checking idlelib, I presume the other non-underscore names you list there are used also.

flavianh · 2019-07-19T22:59:53Z

I have made the requested changes; please review again

bedevere-bot · 2019-07-19T22:59:56Z

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

flavianh · 2019-07-20T09:10:23Z

Had to move __version__ here to abide to PEP8 also

flavianh · 2019-07-20T09:28:12Z

@serhiy-storchaka All your comments should now be addressed!

flavianh · 2019-07-23T12:16:38Z

@terryjreedy @serhiy-storchaka Anything blocking?

terryjreedy

Serhiy is responsible for final details of tkinter/* changes.

The expression `from tkinter import *` actually imports re, enum, sys, as well as tkinter functions and classesreserved for internal use. This commit dismisses most of those names by defining the __all__ variable in accordance with the documentation in tkinter and its submodules.

flavianh · 2019-07-24T09:36:55Z

@terryjreedy I had to remove your commit "Move editor.py changes to another PR for full backport" because it was undoing your own changes when I rebased on master ;)

terryjreedy · 2019-07-25T20:11:04Z

Serhiy, as near as I can tell, this is ready to go. If you agree, it would be nice to get it into a3, due next Monday.

serhiy-storchaka · 2019-07-25T20:24:48Z

 import tkinter

+__version__ = "0.9"
+__all__ = ["NORMAL", "ROMAN", "BOLD", "ITALIC", "nametofont", "Font", "families",


I would break a line after "ITALIC".

Sure thing. What is your rule for line breaks? I'd like to automate it in my IDE

It is hard to automate. Semantically the first four names in this list are names of constants.

serhiy-storchaka · 2019-07-25T20:25:58Z


 from tkinter.commondialog import Dialog

+__all__ = ["showinfo", "showwarning", "showerror", "askquestion", "askokcancel",


I suggest to break a line after "showerror".

serhiy-storchaka · 2019-07-25T20:29:17Z

 These interfaces were written by Fredrik Lundh, May 1997.
 """
+__all__ = ["FileDialog", "LoadFileDialog", "SaveFileDialog", "Open", "SaveAs",
+           "Directory", "askopenfilename", "asksaveasfilename", "askopenfilenames",


This line is longer then PEP 8 limit. I would format the list like:

__all__ = ["FileDialog", "LoadFileDialog", "SaveFileDialog", "Open", "SaveAs", "Directory", "askopenfilename", "asksaveasfilename", "askopenfilenames", "askopenfile", "askopenfiles", "asksaveasfile", "askdirectory"]

Added the 79-character line length to my IDE's configuration for cPython

flavianh · 2019-07-25T21:08:08Z

@serhiy-storchaka Changes done ;)

flavianh · 2019-07-25T21:34:56Z

@terryjreedy up to you :)

terryjreedy · 2019-07-26T01:35:04Z

It doesn't really matter which of us merges. I presume that this should not be backported to 3.8 because b3 is too late for new features, even all. There must be other code that will break with this change, just like IDLE did. Thank you for sticking with this.

the-knights-who-say-ni added the CLA signed label Jul 19, 2019

bedevere-bot added the awaiting review label Jul 19, 2019

terryjreedy requested a review from serhiy-storchaka July 19, 2019 16:42

flavianh force-pushed the bpo-29446-from-tkinter-import-all branch 2 times, most recently from 55f563e to d3916dd Compare July 19, 2019 17:34

flavianh requested a review from terryjreedy as a code owner July 19, 2019 17:34

flavianh force-pushed the bpo-29446-from-tkinter-import-all branch from 7cf43f0 to 76c8d35 Compare July 19, 2019 17:41

serhiy-storchaka reviewed Jul 19, 2019

View reviewed changes

terryjreedy requested changes Jul 19, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Jul 19, 2019

bedevere-bot added the awaiting changes label Jul 19, 2019

terryjreedy reviewed Jul 19, 2019

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2019-07-19-16-06-48.bpo-29446.iXGuoi.rst Outdated

flavianh force-pushed the bpo-29446-from-tkinter-import-all branch from 94460f3 to 7037f85 Compare July 19, 2019 22:41

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 19, 2019

serhiy-storchaka reviewed Jul 20, 2019

View reviewed changes

flavianh force-pushed the bpo-29446-from-tkinter-import-all branch 2 times, most recently from 0cdd92a to 5180d87 Compare July 20, 2019 09:09

flavianh commented Jul 20, 2019

View reviewed changes

terryjreedy assigned serhiy-storchaka Jul 23, 2019

terryjreedy reviewed Jul 23, 2019

View reviewed changes

Comment thread Lib/idlelib/editor.py Outdated

terryjreedy reviewed Jul 23, 2019

View reviewed changes

Comment thread Lib/tkinter/filedialog.py Outdated

Flavian Hautbois and others added 2 commits July 24, 2019 11:11

📜🤖 Added by blurb_it.

f4188aa

Adding acknowledgment

fc260c4

flavianh force-pushed the bpo-29446-from-tkinter-import-all branch from 7ea2ce5 to 2345535 Compare July 24, 2019 09:13

Use explicit imports in filedialog.py

e9cae4f

flavianh force-pushed the bpo-29446-from-tkinter-import-all branch from 2345535 to e9cae4f Compare July 24, 2019 09:36

serhiy-storchaka reviewed Jul 25, 2019

View reviewed changes

Add missing __all__ and reformat others

6883c8e

serhiy-storchaka approved these changes Jul 25, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 25, 2019

terryjreedy merged commit 76b6451 into python:master Jul 26, 2019

bedevere-bot removed the awaiting merge label Jul 26, 2019

serhiy-storchaka removed their assignment Dec 29, 2020

This was referenced Dec 13, 2024

Explicit imports in tkinter.simpledialog #127930

Closed

gh-127930: use explicit imports in tkinter.simpledialog #127931

Merged


		from tkinter.commondialog import Dialog

		__all__ = ["showinfo", "showwarning", "showerror", "askquestion", "askokcancel",

Uh oh!

Conversation

flavianh commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flavianh Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flavianh Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flavianh Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

terryjreedy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 19, 2019

Uh oh!

Uh oh!

terryjreedy commented Jul 19, 2019

Uh oh!

flavianh commented Jul 19, 2019

Uh oh!

bedevere-bot commented Jul 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flavianh Jul 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flavianh commented Jul 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flavianh commented Jul 23, 2019

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flavianh commented Jul 24, 2019

Uh oh!

terryjreedy commented Jul 25, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flavianh commented Jul 19, 2019 •

edited

Loading

flavianh Jul 19, 2019 •

edited

Loading

flavianh Jul 19, 2019 •

edited

Loading

flavianh Jul 19, 2019 •

edited

Loading

terryjreedy left a comment •

edited

Loading

flavianh Jul 20, 2019 •

edited

Loading

flavianh commented Jul 20, 2019 •

edited

Loading

terryjreedy commented Jul 26, 2019 •

edited

Loading