IDLE: Modernize configdialog code.

[terryjreedy]

BPO	30728
Nosy	@terryjreedy, @mlouielu, @csabella
PRs	bpo-30728: IDLE: Refactor configdialog to PEP8 names and add docstrings #2307 [3.6] bpo-30728: IDLE: Refactor configdialog to PEP8 names (GH-2307) #2421
Dependencies	bpo-28523: Idlelib.configdialog: use 'color' insteadof 'colour' bpo-30777: IDLE: configdialog -- add docstrings and improve comments bpo-30779: IDLE: configdialog -- factor out Changes class bpo-30780: IDLE: configdialog - add tests for ConfigDialog GUI. bpo-30781: IDLE: configdialog -- switch to ttk widgets.
Files	configdialog_custom_name_changes.txt: Changes that do not follow normal lowercasing rule

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/terryjreedy'
closed_at = None
created_at = <Date 2017-06-21.23:36:55.022>
labels = ['expert-IDLE', 'type-feature', '3.7']
title = 'IDLE: Modernize configdialog code.'
updated_at = <Date 2017-07-24.16:03:13.768>
user = 'https://github.com/terryjreedy'

bugs.python.org fields:

activity = <Date 2017-07-24.16:03:13.768>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = False
closed_date = None
closer = None
components = ['IDLE']
creation = <Date 2017-06-21.23:36:55.022>
creator = 'terry.reedy'
dependencies = ['28523', '30777', '30779', '30780', '30781']
files = ['46970']
hgrepos = []
issue_num = 30728
keywords = []
message_count = 8.0
messages = ['296603', '296613', '296624', '296652', '296743', '296949', '296980', '296981']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'louielu', 'cheryl.sabella']
pr_nums = ['2307', '2421']
priority = 'normal'
resolution = None
stage = 'test needed'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue30728'
versions = ['Python 3.6', 'Python 3.7']

[terryjreedy]

Depending on the file, modernizing includes:

• Add docstrings.
• Change non-class names with caps to PEP-8 no-caps names;
  this often means, for example, changing embedded 'A' to '_a'.
• Make most comments be sentences, like this sentence.
• Review and possibly change overly cryptic existing PEP-8 names.
• Switch to ttk widgets and revise imports; 'from tkinter import *'
  becomes 'from tkinter(.ttk) import item1, item2, ...';
  use (...\n...) for multiple lines.
• Split a toplevel class into a window class and a frame class.
• Use modern code idioms and features up to current maintenance version.
• Add tests, which are essential for checking correctness of above.

For a large file like configdialog.py, I would like a separate PR or even issue for each item above. Of necessity, name changes must be done in test_configdialog.py, and it should be included in most patches for this issue. For some files, other consumers will also need patching. I don't think that modules other than test_configdialog access much of anything beside ConfigDialog, which name we are not presently changing.

Cheryl's original patch for this issue combined conversion to PEP-8 names, revision of PEP-8 names (including existing bpo-28523 'colour' to 'color'), and tkk and import conversion (and an unrelated typo correction in another file). Louie suggested adding comment revisions. At this point, I decided that this would be too much for one patch and that I would prefer more numerous patches that would be easier to review and test. (Louie's comments should be the basis for a separate PR.) bpo-28523 can be made a dependency of this issue and expanded to other name revisions.

The order of changes is somewhat arbitrary, except that docstrings and comments do not need tests, and can be helpful for writing tests. Tests of some type are otherwise needed for the other steps. This presents a problem when code changes are needed to write tests. What is true is that each patch needs to apply to the current file, and once applied, should be backported immediately. (Out of order backports can 'work' but produce a wrong result. This happened already.)

The problem of multiple patches to the same file possibly breaking each other includes exist patches on the tracker. When we changed textview, there were no outstanding patches that I know of. For help_about, there is an existing patch, but I did not want to apply it completely as is. Pieces of it will have to be adapted as needed. For config_key, there are at least 2 patches that may be ready-to-go or close to it. So I want to test those before writing additional patches to modernize config_key.

I will review current config-dialog issues to see which have patches and which might be quick to apply.

[terryjreedy]

I moved PR 2307 here. I made trivial PR 2322 for the config_key typo.

Cheryl, how did you do the name changes? Did you prepare a rename mapping or script that could be applied to other patch files? If so, please upload.

If sensibly possible, I would like rename patches to start with a rename list that can be reviewed before making a patch. For example, bpo-24225 began with a patch first, followed by the list of renames in msg243572. I rejected some of the renames as too mechanical and posted https://bugs.python.org/file42678/%40newnames.txt for review. It would have saved me a couple of hours if the OP had done the same before making the patch.

[csabella]

Hi Terry,

I don't have a script for the renaming. Since I was reading the code for the docstrings at the same time, I just made the changes as I went along. However, my primary intent is to save you time and work, not cause more, so I will prepare the information requested. Is the best format a txt file? Or should it be a dictionary mapping that can be fed into a replace? Please let me know.

I'm still trying to figure out what changes to include in a pull request, so it's helpful to have these steps defined. I realize I shouldn't have included the change to the keys file with this request, but I was just thinking that I didn't want to forget about it and I included it with the add.

For PR2307, should I separate the docstrings into a separate PR or do you want to let this one go as is?

Would it be helpful to post msg296603 and the roadmap on idle-dev? I had also worked on this as part of the roadmap for modernizing code. I don't really have a clear picture of the current patches that you want to apply. You're so well organized, maybe you can share your specific goals for existing patches? For example, if you know there are 2 config_keys patches that are ready to go, but might need some testing, then I can help with that (if there was some way for me to help). That way I can focus on the parts of the project that would help you most without causing undue extra work. I saw the roadmap as a TODO list of things that needed patches without realizing some of the issues already had patches.

Thanks!

[terryjreedy]

Yes, I intend to add the expanded list, further edited, to the roadmap, or something not tied to one issue. I should add changing messagebox and font imports.

Except for switching to ttk widgets, the changes listed are invisible to users. The purpose of these changes is to make it easier to make changes that are visible, that improve the user experience, without introducing regressions (the second reason tests are needed). So once we make the invisible changes in a file, we should look at making visible changes in the same file.

Having re-written help_about *and* the tests, we can and logically should proceed to visible changes. I reviewed the patch for bpo-24813 and identified 6 independent changes. I want to do 1, 3, 4, and likely 2. I added a note for you there.

For existing patches, new tests help, code changes hinder. We face tradeoffs and chicken-and-egg problems constantly. For config_key, I decided that patches for both bpo-6739 and bpo-21519 should be applied, with revision and tests, in that order. I added a note to bpo-6739 about making a PR. Try it if you like.

I think this issue should remain on hold until I look more at existing patches.

The 'F3' typo/bug was noted in 2014 in msg220625 but was not fixed or added to the existing patch. Doing something was better ;-). In the future, in a similar situation, you could add a trivial PR and request me to review.

[terryjreedy]

I reviewed IDLE issues with patches. I will post my updated issues list on the roadmap issue, bpo-30422. Of relevance to this issue is that config related patches are split between config, configdialog, and config_key, and limited to 3 or 4 each.

I decided that we should start with a reduced version of PR 2307 focused on the caps names to no-caps name changes. I evaluated other proposed changes on their likelihood of creating merge conflicts. Details are included in a new review of PR 2307.

It should be too difficult to make the same caps to no-caps changes in existing patch files, whether by hand or script. First consider a TitleCase name, beginning with a cap. We assume that it is a class or a name from another module unless discovered or indicated otherwise. Any name following 'def' is not a class. A file can be scanned once to find all function names defined in a module. I did not see any local or non-function attribute TitleCase names.

We can assume that camelCase names are not class names and should change unless specified otherwise. The change can be be dict lookup or the repacement rule: 'X' to '_X'. Attached is a file specifying camelCase names that should not be changed or that have a custom replacement. TitleCase names can also have custom replacements.

[terryjreedy]

New changeset bac7d33 by terryjreedy (csabella) in branch 'master':
bpo-30728: IDLE: Refactor configdialog to PEP-8 names (bpo-2307)
bac7d33

[terryjreedy]

I decided that most other changes should be separate issues that are dependencies of this one.

30779 Docstrings and comments
28523 Colour to color
30779 Factor out Changes class
30780 Test GUI - depends on 30779
30781 Switch to ttk - depends on 30780

[terryjreedy]

New changeset 938e738 by terryjreedy in branch '3.6':
[3.6] bpo-30728: IDLE: Refactor configdialog to PEP-8 names (GH-2307) (bpo-2421)
938e738

[erlend-aasland]

The bpo list in #74913 (comment) corresponds to these GH issues:

• bpo-30779: IDLE: configdialog -- factor out Changes class #74962
• bpo-28523: Idlelib.configdialog: use 'color' insteadof 'colour' #72709
• bpo-30780: IDLE: configdialog - add tests for ConfigDialog GUI. #74963
• bpo-30781: IDLE: configdialog -- switch to ttk widgets. #74964

Note that bpo-30779 was originally listed twice.

Are there more work to be done here, or can we close this issue?

[terryjreedy]

I cannot immediately tell. I should make a Config tasks issue and add anything here not covered elsewhere and then close this. And maybe summarize done tasks.

[erlend-aasland]

I should make a Config tasks issue and add anything here not covered elsewhere and then close this. And maybe summarize done tasks.

I don't see the need for another meta-issue; there's already a Config topic in the IDLE GitHub project for such purposes. If that is not sufficient, what is lacking?

[terryjreedy]

Since there is a narrow config topic, ideas can go there.

[erlend-aasland]

Since there is a narrow config topic, ideas can go there.

Sounds good. One can make use of the issue drafts in GitHub projects for vague ideas that have yet to be manifested into an actionable issue. See our sqlite3 GitHub project for inspiration.

[erlend-aasland]

Anyways, I suggest to close this and use the Config topic in the IDLE GitHub Project for managing further enhancements. The use of draft issues can hopefully be used to reduce scope creep in existing issues.