◐ Shell
reader mode source ↗
Skip to content

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

Merged
terryjreedy merged 5 commits into
python:masterfrom
csabella:configdialog
Jun 26, 2017
Merged

bpo-30728: IDLE: Refactor configdialog to PEP8 names and add docstrings#2307
terryjreedy merged 5 commits into
python:masterfrom
csabella:configdialog

Conversation

@csabella

@csabella csabella commented Jun 20, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor
  1. Changed CamelCase names to PEP8.
  2. Changefrom tkinter import * to import each name.
  3. Changed 'colour' to 'color' as mentioned in a bug report.
  4. Added docstrings.
  5. Typo in config_key fixed.

mlouielu
mlouielu suggested changes Jun 21, 2017
View reviewed changes

@mlouielu mlouielu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

mlouielu
mlouielu suggested changes Jun 21, 2017
View reviewed changes

@mlouielu mlouielu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

18 hidden conversations Load more…
terryjreedy
terryjreedy requested changes Jun 21, 2017
View reviewed changes

@terryjreedy terryjreedy left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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.

5 hidden conversations Load more…
@terryjreedy terryjreedy changed the title bpo-27388: IDLE: Refactor configdialog to PEP8 names and add docstrings Jun 21, 2017
terryjreedy
terryjreedy requested changes Jun 23, 2017
View reviewed changes

@terryjreedy terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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.

8 hidden conversations Load more…
@csabella

csabella commented Jun 25, 2017

Copy link
Copy Markdown
Contributor Author

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
terryjreedy reviewed Jun 26, 2017
View reviewed changes

@terryjreedy terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

@terryjreedy

terryjreedy commented Jun 26, 2017

Copy link
Copy Markdown
Member

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. )

@terryjreedy terryjreedy merged commit bac7d33 into python:master Jun 26, 2017
terryjreedy pushed a commit to terryjreedy/cpython that referenced this pull request Jun 26, 2017
@csabella @terryjreedy
[3.6] bpo-30728: IDLE: Refactor configdialog to PEP8 names (pythonGH-… …
fbdc863 
…2307)

Also, change '*' in the tkinter import to an explicit list of names.
Patch by Cheryl Sabella.
(cherry picked from commit bac7d33)
@csabella csabella deleted the configdialog branch June 26, 2017 22:46
terryjreedy added a commit that referenced this pull request Jun 27, 2017
@terryjreedy
[3.6] bpo-30728: IDLE: Refactor configdialog to PEP8 names (GH-2307) (#… …
938e738 
…2421)

Also, change '*' in the tkinter import to an explicit list of names.
Patch by Cheryl Sabella.
(cherry picked from commit bac7d33)
@mlouielu mlouielu mentioned this pull request Jul 11, 2017
Closed
@terryjreedy terryjreedy mentioned this pull request Jul 29, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terryjreedy terryjreedy terryjreedy approved these changes

+1 more reviewer

@mlouielu mlouielu mlouielu requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@csabella @terryjreedy @mlouielu @the-knights-who-say-ni