classification
Title: IDLE: Modernize configdialog code.
Type: enhancement Stage: test needed
Components: IDLE Versions: Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: 28523 30777 30779 30780 30781 Superseder:
Assigned To: terry.reedy Nosy List: cheryl.sabella, louielu, terry.reedy
Priority: normal Keywords:

Created on 2017-06-21 23:36 by terry.reedy, last changed 2017-07-24 16:03 by terry.reedy.

Files
File name Uploaded Description Edit
configdialog_custom_name_changes.txt terry.reedy, 2017-06-23 22:47 Changes that do not follow normal lowercasing rule
Pull Requests
URL Status Linked Edit
PR 2307 merged cheryl.sabella, 2017-06-21 23:38
PR 2421 merged terry.reedy, 2017-06-26 21:49
Messages (8)
msg296603 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-21 23:36
Depending on the file, modernizing includes:
* Add docstrings.
* Change non-class names with caps to PEP8 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 PEP8 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 pep8 names, revision of pep8 names (including existing #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.) Issue 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.
msg296613 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-22 02:49
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, #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.
msg296624 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-06-22 11:51
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!
msg296652 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-22 20:03
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 #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 #6739 and #21519 should be applied, with revision and tests, in that order.  I added a note to #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.
msg296743 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-23 22:47
I reviewed IDLE issues with patches.  I will post my updated issues list on the roadmap issue, #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.
msg296949 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-26 21:46
New changeset bac7d3363b099d0cdef3e541f8581859edfddc85 by terryjreedy (csabella) in branch 'master':
bpo-30728: IDLE: Refactor configdialog to PEP8 names (#2307)
https://github.com/python/cpython/commit/bac7d3363b099d0cdef3e541f8581859edfddc85
msg296980 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-27 00:46
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
msg296981 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-27 00:48
New changeset 938e73809d10f6073c85ecd330c88a85c6095530 by terryjreedy in branch '3.6':
[3.6] bpo-30728: IDLE: Refactor configdialog to PEP8 names (GH-2307) (#2421)
https://github.com/python/cpython/commit/938e73809d10f6073c85ecd330c88a85c6095530
History
Date User Action Args
2017-07-24 16:03:13terry.reedysetdependencies: - [2.7] test_bsddb3 crash on x86 Windows7 2.7
2017-06-27 00:48:41terry.reedysetmessages: + msg296981
2017-06-27 00:46:41terry.reedysetassignee: terry.reedy
dependencies: + Idlelib.configdialog: use 'color' insteadof 'colour', IDLE: configdialog -- add docstrings and improve comments, [2.7] test_bsddb3 crash on x86 Windows7 2.7, IDLE: configdialog -- factor out Changes class, IDLE: configdialog - add tests for ConfigDialog GUI., IDLE: configdialog -- switch to ttk widgets.
messages: + msg296980
components: + IDLE
2017-06-26 21:49:51terry.reedysetpull_requests: + pull_request2471
2017-06-26 21:46:29terry.reedysetmessages: + msg296949
2017-06-23 22:47:53terry.reedysetfiles: + configdialog_custom_name_changes.txt

messages: + msg296743
2017-06-22 20:03:07terry.reedysetmessages: + msg296652
2017-06-22 11:51:21cheryl.sabellasetmessages: + msg296624
2017-06-22 02:49:03terry.reedysetmessages: + msg296613
2017-06-21 23:38:12cheryl.sabellasetpull_requests: + pull_request2370
2017-06-21 23:36:55terry.reedycreate