bpo-35660: Fix imports in idlelib.window#11434
Conversation
* sys was being imported through the *, so also added an import sys.
| @@ -0,0 +1 @@ | |||
| Remove use of `from tkinter import *` from window.py. | |||
There was a problem hiding this comment.
Use double backquotes (though I doubt a news entry is wanted for such a change).
There was a problem hiding this comment.
I think a news entry is not wanted. This is merely a refactoring, it should not cause any user visible effect.
There was a problem hiding this comment.
I am not a fan of markup in blurbs, which get copied to idlelib NEWS, and I don't this it is needed here.
My understanding of the general policy is that a) if a change is non-trivial, there should be an issue, and b) if there is a non-trivial issue, there should be at least a short blurb. To me, the only code change that is truly trivial is something that cannot affect users -- which is a change to comments. Simple spelling or grammar changes to docstrings and docs also qualify. I don't see anything in the devguide specifically addressing these points.
This issue fixes a style violation and a bug. Like any refactoring, it could cause a user-visible effect if not done right. A needed name could have been omitted. The sys import bug could have been missed (there is no test). The audience for blurbs includes future maintainers, including me. I occasionally use the IDLE blurb list to check on what has been when. I agree that this is a borderline case, but the last point is why I decided to keep at least a short note here.
There was a problem hiding this comment.
I think that news entries are for users, and future maintainers can use the VCS log.
The Lib/idlelib/window.py file is an implementation detail, it should not be directly imported by user code.
There was a problem hiding this comment.
The log is not quite the same. Anyway, there is nothing special about window.py. With a couple of exceptions, there is no official support for importing any of idlelib files.
If you think my handling of news items (and possibly that of predecessors*) is and has been wrong, please open a pydev issue stating what you think the policy is, or should be. You might include a review of a month or two of all blurbs stating which ones you think should be deleted.
- I looked at idlelib/NEWS2.txt to get some idea of what KBK did. There are several 'cleanup X' issues. Some have issue numbers, some not. "cleanup TextView", #1718043, appears to be internal only. "cleanup EditorWindow.close" sounds like it might be, but there is no issue. The Sourceforge SVN/r##### links on issues are dead (404) for me. I cannot find either patch in the git log. Perhaps I am not using it correctly.
terryjreedy
changed the title
Anyone who wants details can check the issue, where I added the point about the sys import bug.
|
Thanks @csabella for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
@terryjreedy: Please replace |
|
GH-11449 is a backport of this pull request to the 3.7 branch. |
* bpo-35660: IDLE: Remove * import from window.py * sys was being imported through the *, so also added an import sys. * Update 2019-01-04-19-14-29.bpo-35660.hMxI7N.rst Anyone who wants details can check the issue, where I added the point about the sys import bug. (cherry picked from commit 11303dd) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
* bpo-35660: IDLE: Remove * import from window.py * sys was being imported through the *, so also added an import sys. * Update 2019-01-04-19-14-29.bpo-35660.hMxI7N.rst Anyone who wants details can check the issue, where I added the point about the sys import bug. (cherry picked from commit 11303dd) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
7 participants
*, so also added animport sys.https://bugs.python.org/issue35660