Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.1k
bpo-30728: IDLE: Refactor configdialog to PEP8 names and add docstrings#2307
bpo-30728: IDLE: Refactor configdialog to PEP8 names and add docstrings#2307terryjreedy merged 5 commits intopython:masterfrom
Conversation
Lib/idlelib/config_key.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be in another issue (?), or do you want to PEP8 them, too? (I think this can be PEP8, too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My initial inclination was to leave this as is. But I found two issues patching config_key (as keybindingDialog):bpo-6739 andbpo-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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove. Fixed in a trivial PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Explicit wrote downroot andconfigure, too.
e.g.root = None,configure = None and add two blank line between them and class TestDialog
mlouielu left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My computer dead at configdialog.py diff :(, I'll seperate by review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove blank line between FontTabTest and setUp or add the docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is also an extra blank above tearDownModule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
dfont,dsize,dbold, may changed explicited todefault_font,default_size,default_bold since it is testing about default settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Change the names. I will rewrite more later.
mlouielu left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I leave some point, the comment first word should be capitalized.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
According toPEP328, this import should be used parentheses import like this:
from tkinter import (Toplevel, Frame, LabelFrame, Listbox, Label, Button, Entry, Text, Scale, Radiobutton, Checkbutton, Canvas)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree that this would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Revise import revision as suggested and leave in patch.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not add the variable name but add a blank line betweenself.geometry and comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just remove 'Theme Elements. ' and add a blank.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For first patch, undo all changes to comments. We will revise them as per the suggestions in a follow-up patch.
Leave code changes ininit.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Comment first word should be capitalizeLoad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As said above, table all suggested changes to comments for another patch. Changing comments can create merge conflicts that cannot be mechanically fixed.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This seems not used, but make it capitalized, and add a blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ishttps://bugs.python.org/issue27620. I added a note there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Leave uncommenting for a later patch.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
CapitalizedAvoid
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
CapitalizedUser
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
CapitalizedUser
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
CapitalizedCreate,Current
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
CapitalizedSet
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
CapitalizedCreates
terryjreedy left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.Lib/idlelib/config_key.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My initial inclination was to leave this as is. But I found two issues patching config_key (as keybindingDialog):bpo-6739 andbpo-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.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree that this would be better.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just remove 'Theme Elements. ' and add a blank.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
# Dismiss dialog without saving.# Apply changes and save.
`# Open context help.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I might have once changedtype totyp. Many consider reusing builtin names a bad idea in general. With syntax highlighting, it becomes visually distracting, misleadingly markingtype as different from the other names.type_ could be used iftype is really the proper name, but it is not here. config module usestype as a parameter name where it meansbool orint, (butcls 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 becomefile.
Changing existing pep8 names is not really part of this patch.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good idea.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is also an extra blank above tearDownModule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Revise import revision as suggested and leave in patch.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For first patch, undo all changes to comments. We will revise them as per the suggestions in a follow-up patch.
Leave code changes ininit.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As said above, table all suggested changes to comments for another patch. Changing comments can create merge conflicts that cannot be mechanically fixed.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Leave uncommenting for a later patch.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Undo code change; we will review code later.
textAndTags => text_and_tags, by rule
txTa => texttag, as temporary custom replacement
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Uncomment for now.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Change the names. I will rewrite more later.
Lib/idlelib/configdialog.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But leave alone for now.
csabella commentedJun 25, 2017
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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If no problems arise when I test the dialog, I will merge this.
terryjreedy commentedJun 26, 2017
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. ) |
Uh oh!
There was an error while loading.Please reload this page.
from tkinter import *to import each name.