bpo-30728: IDLE: Refactor configdialog to PEP8 names and add docstrings #2307

csabella · 2017-06-20T22:59:49Z

Changed CamelCase names to PEP8.
Changefrom tkinter import * to import each name.
Changed 'colour' to 'color' as mentioned in a bug report.
Added docstrings.
Typo in config_key fixed.

mlouielu · 2017-06-21T08:30:52Z

Lib/idlelib/config_key.py

@@ -186,7 +186,7 @@ def ClearKeySeq(self):

    def LoadFinalKeyList(self):
        #these tuples are also available for use in validity checks
-        self.functionKeys=('F1','F2','F2','F4','F5','F6','F7','F8','F9',
+        self.functionKeys=('F1','F2','F3','F4','F5','F6','F7','F8','F9',


This should be in another issue (?), or do you want to PEP8 them, too? (I think this can be PEP8, too)

My initial inclination was to leave this as is. But I found two issues patching config_key (as keybindingDialog): bpo-6739 and bpo-21519. (The patches overlap, but are not affected by the above.) With the file name changed in the diffs, I expect the patches would apply cleanly. I would like to verify the bugs, verify the fixes, add tests, and if appropriate, apply these patches, and possibly other more-or-less ready to go patches, before internal renaming.

Remove. Fixed in a trivial PR.

mlouielu · 2017-06-21T08:34:20Z

Lib/idlelib/idle_test/test_configdialog.py

@@ -26,7 +26,7 @@
 # For testing, record args in a list for comparison with expected.
 changes = []


Explicit wrote down root and configure, too.
e.g. root = None, configure = None and add two blank line between them and class TestDialog

mlouielu

My computer dead at configdialog.py diff :(, I'll seperate by review.

mlouielu · 2017-06-21T08:35:22Z

Lib/idlelib/idle_test/test_configdialog.py

@@ -58,21 +58,21 @@ def test_font(self):
        dfont = idleConf.GetFont(root, 'main', 'EditorWindow')
        dsize = str(dfont[1])
        dbold = dfont[2] == 'bold'
-        configure.fontName.set('Test Font')
+        configure.font_name.set('Test Font')


Remove blank line between FontTabTest and setUp or add the docstring

There is also an extra blank above tearDownModule.

mlouielu · 2017-06-21T08:36:54Z

Lib/idlelib/idle_test/test_configdialog.py

@@ -58,21 +58,21 @@ def test_font(self):
        dfont = idleConf.GetFont(root, 'main', 'EditorWindow')
        dsize = str(dfont[1])
        dbold = dfont[2] == 'bold'
-        configure.fontName.set('Test Font')
+        configure.font_name.set('Test Font')


dfont, dsize, dbold, may changed explicited to default_font, default_size, default_bold since it is testing about default settings.

The function is actually about testing changes to 1 of the 3 items that define a font. I need to expand the name of the function and add a docstring that explains the issue and why the expected is what it is and what could go wrong. The short names are my laziness. If the longer names make the code easier for others to read, I will use them.

Change the names. I will rewrite more later.

mlouielu

I leave some point, the comment first word should be capitalized.

mlouielu · 2017-06-21T08:47:25Z

Lib/idlelib/configdialog.py

+from tkinter import StringVar, BooleanVar, IntVar
+from tkinter import TOP, BOTTOM, RIGHT, LEFT, SOLID, GROOVE, NORMAL, DISABLED
+from tkinter import NONE, BOTH, X, Y, W, E, EW, NS, NSEW, NW
+from tkinter import HORIZONTAL, VERTICAL, ANCHOR, END


According to PEP328, this import should be used parentheses import like this:

from tkinter import (Toplevel, Frame, LabelFrame, Listbox, Label, Button, Entry, Text, Scale, Radiobutton, Checkbutton, Canvas)

I agree that this would be better.

Revise import revision as suggested and leave in patch.

mlouielu · 2017-06-21T08:50:24Z

Lib/idlelib/configdialog.py

+        x = parent.winfo_rootx() + 20
+        y = parent.winfo_rooty() + (30 if not _htest else 150)
+        self.geometry(f'+{x}+{y}')
+        # theme_elements. Each theme element key is its display name.


Not add the variable name but add a blank line between self.geometry and comment.

Just remove 'Theme Elements. ' and add a blank.

For first patch, undo all changes to comments. We will revise them as per the suggestions in a follow-up patch.
Leave code changes in init.

mlouielu · 2017-06-21T08:51:00Z

Lib/idlelib/configdialog.py

-        self.ResetChangedItems() #load initial values in changed items dict
-        self.CreateWidgets()
-        self.resizable(height=FALSE, width=FALSE)
+        self.reset_changed_items()  # load initial values in changed items dict


Comment first word should be capitalize Load

In general, "comments should be complete sentences", with a space after '#', first word capitalized, and a period at the ending. # Load .... dict. Though contrary to much programmer practice, including my own once, I prefer to follow this.

Phrases are allowed (capitalized), and if short, the period can omitted. There are occasions where short phrases are appropriate, but they are far fewer than their use.

As said above, table all suggested changes to comments for another patch. Changing comments can create merge conflicts that can not be mechanically fixed.

mlouielu · 2017-06-21T08:52:07Z

Lib/idlelib/configdialog.py

-        self.AttachVarCallbacks() #avoid callbacks during LoadConfigs
+        self.protocol("WM_DELETE_WINDOW", self.cancel)
+        self.tab_pages.focus_set()
+        # key bindings for this dialog


This seems not used, but make it capitalized, and add a blank line.

Some people believe that unused code == dead code and should be removed rather than be commented out indefinitely. At some point we should decide. Let's change this to
`# XXX Decide whether to keep or delete these key bindings.

Thinking about it, we bind Escape elsewhere and should here. F1 for help is standard. ^A for apply is not, but usability guidelines say dialogs should be usable without a mouse. On there other hand, Tab and Return should work. Future project: We should add 'IDLE Dialog Conventions' to README and follow consistently. Leave code commented out for now.

Keys for mouse actions is https://bugs.python.org/issue27620. I added a note there.

Leave uncommenting for a later patch.

mlouielu · 2017-06-21T08:52:33Z

Lib/idlelib/configdialog.py

+        # self.bind('<Alt-a>', self.Apply) #apply changes, save
+        # self.bind('<F1>', self.Help) #context help
+        self.load_configs()
+        self.attach_var_callbacks()  # avoid callbacks during load_configs


Capitalized Avoid

mlouielu · 2017-06-21T09:08:20Z

Lib/idlelib/configdialog.py

+                parent=self, title='Pick new color for : ' + target,
+                initialcolor=prev_color)
+        if color_string and (color_string != prev_color):
+            # user didn't cancel, and they chose a new color


Capitalized User

mlouielu · 2017-06-21T09:08:28Z

Lib/idlelib/configdialog.py

-                newTheme = self.GetNewThemeName(message)
-                if not newTheme:  #user cancelled custom theme creation
+                new_theme = self.get_new_theme_name(message)
+                if not new_theme:  # user cancelled custom theme creation


Capitalized User

mlouielu · 2017-06-21T09:08:42Z

Lib/idlelib/configdialog.py

+                else:  # create new custom theme based on previously active theme
+                    self.create_new_theme(new_theme)
+                    self.color.set(color_string)
+            else:  # current theme is user defined


Capitalized Create, Current

mlouielu · 2017-06-21T09:08:54Z

Lib/idlelib/configdialog.py

+    def on_new_color_set(self):
+        "Handle a color change by displaying sample of it on the dialog."
+        new_color = self.color.get()
+        self.frame_color_set.config(bg=new_color)  # set sample


Capitalized Set

mlouielu · 2017-06-21T09:09:10Z

Lib/idlelib/configdialog.py

+        activate the new theme.
+        """
+
+        # creates new custom theme based on the previously active theme,


Capitalized Creates

terryjreedy

Thank you Cheryl for the work this involved and Louie for the non-trivial review. I did not review every change but focused on Louie's comments, as they raise a couple of meta-issues of how much to do in one patch. As a result, I have a better idea how to move forward.

This PR does not belong to #27388. That aside, I think this PR does too much at once and may be pre-mature.

After submitting this, I will open a new issue for modernizing configdialog. Modernizing includes: adding docstrings; changing to PEP8 names; changing most comments to sentences; reviewing and possibly changing overly cryptic existing PEP8 names; and at least metaphorically, adding tests. For a large file like config dialog, I would like a separate PR for each of these.

Louie's comment on config_key got me thinking again about the problem of breaking existing tracker patches, especially any that are ready to go. There were no patches that I know of for textview and I don't intend to apply the existing one for About IDLE as is.

Many of the comments below also appear above as responses to Louie's comments. Most can also be understood as they are.

terryjreedy · 2017-06-21T16:14:58Z

Lib/idlelib/config_key.py

@@ -186,7 +186,7 @@ def ClearKeySeq(self):

    def LoadFinalKeyList(self):
        #these tuples are also available for use in validity checks
-        self.functionKeys=('F1','F2','F2','F4','F5','F6','F7','F8','F9',
+        self.functionKeys=('F1','F2','F3','F4','F5','F6','F7','F8','F9',


My initial inclination was to leave this as is. But I found two issues patching config_key (as keybindingDialog): bpo-6739 and bpo-21519. (The patches overlap, but are not affected by the above.) With the file name changed in the diffs, I expect the patches would apply cleanly. I would like to verify the bugs, verify the fixes, add tests, and if appropriate, apply these patches, and possibly other more-or-less ready to go patches, before internal renaming.

terryjreedy · 2017-06-21T16:16:18Z

Lib/idlelib/configdialog.py

+from tkinter import StringVar, BooleanVar, IntVar
+from tkinter import TOP, BOTTOM, RIGHT, LEFT, SOLID, GROOVE, NORMAL, DISABLED
+from tkinter import NONE, BOTH, X, Y, W, E, EW, NS, NSEW, NW
+from tkinter import HORIZONTAL, VERTICAL, ANCHOR, END


I agree that this would be better.

terryjreedy · 2017-06-21T16:19:09Z

Lib/idlelib/configdialog.py

+        x = parent.winfo_rootx() + 20
+        y = parent.winfo_rooty() + (30 if not _htest else 150)
+        self.geometry(f'+{x}+{y}')
+        # theme_elements. Each theme element key is its display name.


Just remove 'Theme Elements. ' and add a blank.

terryjreedy · 2017-06-21T16:45:21Z

Lib/idlelib/configdialog.py

+        self.protocol("WM_DELETE_WINDOW", self.cancel)
+        self.tab_pages.focus_set()
+        # key bindings for this dialog
+        # self.bind('<Escape>', self.Cancel) #dismiss dialog, no save


# Dismiss dialog without saving.
# Apply changes and save.
`# Open context help.

terryjreedy · 2017-06-21T16:47:13Z

Lib/idlelib/configdialog.py

-        self.ResetChangedItems() #load initial values in changed items dict
-        self.CreateWidgets()
-        self.resizable(height=FALSE, width=FALSE)
+        self.reset_changed_items()  # load initial values in changed items dict


In general, "comments should be complete sentences", with a space after '#', first word capitalized, and a period at the ending. # Load .... dict. Though contrary to much programmer practice, including my own once, I prefer to follow this.

Phrases are allowed (capitalized), and if short, the period can omitted. There are occasions where short phrases are appropriate, but they are far fewer than their use.

terryjreedy · 2017-06-21T18:06:10Z

Lib/idlelib/configdialog.py

+                              'extensions': {}}
+
+    def add_changed_item(self, typ, section, item, value):
+        "Add item/value pair to changed items dictionary for typ and section."


I might have once changed type to typ. Many consider reusing builtin names a bad idea in general. With syntax highlighting, it becomes visually distracting, misleadingly marking type as different from the other names. type_ could be used if type is really the proper name, but it is not here. config module uses type as a parameter name where it means bool or int, (but cls could be used instead). Here, typ is one of the keys in changed_items, defined just above, which correspond to the config files. These are not type-classes at all. typ should become file.

Changing existing pep8 names is not really part of this patch.

terryjreedy · 2017-06-21T18:06:42Z

Lib/idlelib/configdialog.py

+
+    def get_default_items(self):
+        "Return dictionary of default configuration settings."
+        d_items = {'main': {}, 'highlight': {}, 'keys': {}, 'extensions': {}}


terryjreedy · 2017-06-21T18:14:31Z

Lib/idlelib/configdialog.py

+        """
+        list_index = self.list_bindings.index(ANCHOR)
+        binding = self.list_bindings.get(list_index)
+        bind_name = binding.split()[0]  # first part, up to first space


Or # Get first part, up to first space.
Or delete, as it seems pretty obvious. x.split()[k] is a common idiom.
Or leave comments alone in this PR and revise them to PEP8 standards in a separate PR.

terryjreedy · 2017-06-21T18:37:43Z

Lib/idlelib/idle_test/test_configdialog.py

@@ -58,21 +58,21 @@ def test_font(self):
        dfont = idleConf.GetFont(root, 'main', 'EditorWindow')
        dsize = str(dfont[1])
        dbold = dfont[2] == 'bold'
-        configure.fontName.set('Test Font')
+        configure.font_name.set('Test Font')


There is also an extra blank above tearDownModule.

terryjreedy · 2017-06-21T18:49:02Z

Lib/idlelib/idle_test/test_configdialog.py

@@ -58,21 +58,21 @@ def test_font(self):
        dfont = idleConf.GetFont(root, 'main', 'EditorWindow')
        dsize = str(dfont[1])
        dbold = dfont[2] == 'bold'
-        configure.fontName.set('Test Font')
+        configure.font_name.set('Test Font')


The function is actually about testing changes to 1 of the 3 items that define a font. I need to expand the name of the function and add a docstring that explains the issue and why the expected is what it is and what could go wrong. The short names are my laziness. If the longer names make the code easier for others to read, I will use them.

terryjreedy

After reviewing existing patches and reviewing this one and thinking about my experience with merges, I decided what to revert or delay for a future patch, what to keep for this one (most of it), or add to this one Just a few things), with an eye to minimizing work over the next several patches for configdialog. More on the issue.

terryjreedy · 2017-06-23T20:14:11Z

Lib/idlelib/configdialog.py

+from tkinter import StringVar, BooleanVar, IntVar
+from tkinter import TOP, BOTTOM, RIGHT, LEFT, SOLID, GROOVE, NORMAL, DISABLED
+from tkinter import NONE, BOTH, X, Y, W, E, EW, NS, NSEW, NW
+from tkinter import HORIZONTAL, VERTICAL, ANCHOR, END


Revise import revision as suggested and leave in patch.

terryjreedy · 2017-06-23T20:16:41Z

Lib/idlelib/configdialog.py

+        x = parent.winfo_rootx() + 20
+        y = parent.winfo_rooty() + (30 if not _htest else 150)
+        self.geometry(f'+{x}+{y}')
+        # theme_elements. Each theme element key is its display name.


For first patch, undo all changes to comments. We will revise them as per the suggestions in a follow-up patch.
Leave code changes in init.

terryjreedy · 2017-06-23T20:18:06Z

Lib/idlelib/configdialog.py

-        self.ResetChangedItems() #load initial values in changed items dict
-        self.CreateWidgets()
-        self.resizable(height=FALSE, width=FALSE)
+        self.reset_changed_items()  # load initial values in changed items dict


As said above, table all suggested changes to comments for another patch. Changing comments can create merge conflicts that can not be mechanically fixed.

terryjreedy · 2017-06-23T20:20:14Z

Lib/idlelib/configdialog.py

-        self.AttachVarCallbacks() #avoid callbacks during LoadConfigs
+        self.protocol("WM_DELETE_WINDOW", self.cancel)
+        self.tab_pages.focus_set()
+        # key bindings for this dialog


Leave uncommenting for a later patch.

terryjreedy · 2017-06-23T20:25:03Z

Lib/idlelib/configdialog.py

-        for txTa in textAndTags:
-            text.insert(END, txTa[0], txTa[1])
-        for element in self.themeElements:
+        for txt, tag in txt_and_tags:


Undo code change; we will review code later.
textAndTags => text_and_tags, by rule
txTa => texttag, as temporary custom replacement

terryjreedy · 2017-06-23T21:04:59Z

Lib/idlelib/configdialog.py

        reselect = 0
-        newKeySet = 0
-        if self.listBindings.curselection():
+        # new_key_set = 0


Uncomment for now.

terryjreedy · 2017-06-23T21:27:00Z

Lib/idlelib/configdialog.py

@@ -1251,7 +1425,7 @@ def CreatePageExtensions(self):
                                      selectmode='browse')
        self.extension_list.bind('<<ListboxSelect>>', self.extension_selected)
        scroll = Scrollbar(frame, command=self.extension_list.yview)
-        self.extension_list.yscrollcommand=scroll.set
+        self.extension_list.yscrollcommand = scroll.set


Code changes like should have been left for another patch, but they are here now and very unlikely to cause a problem since the extension tab is relatively new and should not be touched by existing patches.

terryjreedy · 2017-06-23T21:28:45Z

Lib/idlelib/idle_test/test_configdialog.py

@@ -58,21 +58,21 @@ def test_font(self):
        dfont = idleConf.GetFont(root, 'main', 'EditorWindow')
        dsize = str(dfont[1])
        dbold = dfont[2] == 'bold'
-        configure.fontName.set('Test Font')
+        configure.font_name.set('Test Font')


terryjreedy · 2017-06-23T21:29:21Z

Lib/idlelib/idle_test/test_configdialog.py

@@ -58,21 +58,21 @@ def test_font(self):
        dfont = idleConf.GetFont(root, 'main', 'EditorWindow')
        dsize = str(dfont[1])
        dbold = dfont[2] == 'bold'
-        configure.fontName.set('Test Font')
+        configure.font_name.set('Test Font')


Change the names. I will rewrite more later.

terryjreedy · 2017-06-23T21:29:39Z

Lib/idlelib/configdialog.py

-        oldkeys = (
+            # this is a core keybinding
+            self.add_changed_item('keys', key_set, event, value)
+        else:  # this is an extension key binding


But leave alone for now.

csabella · 2017-06-25T17:42:59Z

I've made the changes to configdialog to only include the name changes and the code change in init. I hope I got the file to the correct state for review.

terryjreedy

If no problems arise when I test the dialog, I will merge this.

terryjreedy · 2017-06-26T21:45:22Z

I tested by trying out and applying an example of every possible change on configdialog and checking for the correct change in the user configuration files. (An editor that reloads changed files, Notepad++, was essential for this. )

…2307) Also, change '*' in the tkinter import to an explicit list of names. Patch by Cheryl Sabella. (cherry picked from commit bac7d33)

…2421) Also, change '*' in the tkinter import to an explicit list of names. Patch by Cheryl Sabella. (cherry picked from commit bac7d33)

the-knights-who-say-ni added the CLA signed label Jun 20, 2017

mlouielu reviewed Jun 21, 2017

View reviewed changes

mlouielu suggested changes Jun 21, 2017

View reviewed changes

terryjreedy requested changes Jun 21, 2017

View reviewed changes

terryjreedy changed the title ~~bpo-27388: IDLE: Refactor configdialog to PEP8 names and add docstrings~~ bpo-30728: IDLE: Refactor configdialog to PEP8 names and add docstrings Jun 21, 2017

terryjreedy requested changes Jun 23, 2017

View reviewed changes

csabella added 2 commits June 24, 2017 14:00

bpo-27388: IDLE: Refactor configdialog to PEP8 names and add docstrings

44401cd

bpo-27388: IDLE: Refactor configdialog to PEP8 names and add docstrings

9361976

csabella force-pushed the configdialog branch from 646faad to 9361976 Compare June 24, 2017 18:01

bpo-30728: IDLE: Refactor configdialog to PEP8 names

a264bb1

terryjreedy reviewed Jun 26, 2017

View reviewed changes

Add news entry

c6d5ec4

terryjreedy approved these changes Jun 26, 2017

View reviewed changes

Update 2017-06-26-15-47-13.bpo-30728.qH4TGL.rst

01aa47d

terryjreedy merged commit bac7d33 into python:master Jun 26, 2017

csabella deleted the configdialog branch June 26, 2017 22:46

mlouielu mentioned this pull request Jul 11, 2017

bpo-30900: IDLE: Fix configdialog should use wm_withdraw #2664

Closed

terryjreedy mentioned this pull request Jul 29, 2022

IDLE: Modernize configdialog code. #74913

Open

		@@ -26,7 +26,7 @@
		# For testing, record args in a list for comparison with expected.
		changes = []

bpo-30728: IDLE: Refactor configdialog to PEP8 names and add docstrings #2307

bpo-30728: IDLE: Refactor configdialog to PEP8 names and add docstrings #2307

Conversation

csabella commented Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlouielu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlouielu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella commented Jun 25, 2017

terryjreedy left a comment

Choose a reason for hiding this comment

terryjreedy commented Jun 26, 2017

csabella commented Jun 20, 2017 •

edited

Loading

terryjreedy left a comment •

edited

Loading