bpo-30728: IDLE: Refactor configdialog to PEP8 names and add docstrings by csabella · Pull Request #2307 · python/cpython

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

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

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

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

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

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

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

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

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

Capitalized Avoid

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

Capitalized User

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

Capitalized User

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

Capitalized Create, Current

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

Capitalized Set

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

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

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

I agree that this would be better.

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

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

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

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

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

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

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

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

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

There is also an extra blank above tearDownModule.

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

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

Revise import revision as suggested and leave in patch.

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

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

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

Leave uncommenting for a later patch.

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

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

Uncomment for now.

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

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

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

Change the names. I will rewrite more later.

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

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

Uh oh!

Conversation

csabella commented Jun 20, 2017 • 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!

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!

mlouielu left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

mlouielu left a comment

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!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

csabella commented Jun 20, 2017 •

edited

Loading

terryjreedy left a comment •

edited

Loading