classification
Title: IDLE: add tests for help_about.py
Type: enhancement Stage: commit review
Components: IDLE Versions: Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: 30303 Superseder:
Assigned To: terry.reedy Nosy List: csabella, louielu, terry.reedy
Priority: normal Keywords:

Created on 2017-05-06 05:03 by terry.reedy, last changed 2017-06-10 06:53 by terry.reedy.

Pull Requests
URL Status Linked Edit
PR 1697 merged louielu, 2017-05-21 07:17
PR 1714 merged csabella, 2017-05-22 11:19
PR 1838 merged terry.reedy, 2017-05-28 05:08
PR 2068 closed terry.reedy, 2017-06-10 05:31
PR 2070 merged terry.reedy, 2017-06-10 06:22
Messages (23)
msg293152 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-06 05:03
Initial plan.
1a. Change AboutDialog to mimic query.Query with respect to _utest and suppression of dialog display and waiting.
1b. Create AboutDialog instance.
2a. Change textview.TextViewer as with AboutDialog.  Also change textview functions and AboutDialog methods to pass on _utest.
2b. Simulate keyclicks on buttons and test that root gains appropriate child.  Then destroy it.
3. At some point, remove dead code and change now incorrect encoding comment.

Separate issue: add simulated button click tests to test_textview and other popup dialogs.  Directly calling commands does not test that buttons invoke the right command.
msg293998 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-20 05:01
4a. Change uglyMidcap widget names and CamelCase function names to pep8_conformant names.  Simplify: byline, email, docs, pyver, tkver, idlever, py_license, py_copyright, py_credits, readme, idle_news, idle_credits.

4b. Change CamelCase function names to pep8_conformant names.
ShowIDLEAbout => show_readme, 

5. Add docstrings
For show_xyz functions, "Command for xyz button"? even though not verb?

#30303 implemented 2a, improve textview and tests.

Cheryl: this should be much easier than colorizer.  Let me know if you want to take a stab at at least 4 and 5.
msg294004 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-20 05:43
Follow-up issues.

A. Move code and tests into help.py and test_help.py.
B. Improve content: 1) #25224; 2) remove or update credits;
3) reconsider each item.
C. Improve appearance: 1) ttk widgets; 2) Redo entire look.
msg294058 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-05-20 23:14
This looks like fun!   :-)   I'll let you know if I have any questions.
msg294064 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-05-21 01:37
Terry,

Is there interest in changing the 'from tkinter import *'?
msg294079 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-21 06:43
I take 'fun' to mean you will try something.  And yes, I have thought about the following for all IDLE modules.

6. Replace "from tkinter import *".  It was mainly intended for interactive use. In production IDLE, use either
a. from tkinter  import Tk, Frame, Label, Button, <constants>
b. import tkinter as tk

6a requires more typing in the import statement, but as a replacement, leaves the rest of the code alone.  It documents what widgets will be used in the module.  It allows individual classes to be mocked for a particular test or group of tests.  I know I have done this for the tkinter TypeVar classes.

6b has a short import but requires more typing in the module body, especially when tkinter constants are present, as they are here.

When exploring or writing short, one-off programs, I usually use 'as tk'.  But for IDLE, I may have converted at least one file to 'as tk', but I am now using 'import Tk, ...' and prefer it.

My current thoughts about Tkinter constants versus string literals, such as BOTH versus 'both': The two forms by themselves are about equally easy to type.  The CAPS form is shorter but louder, whereas to me they are minor items that should not be loud. The constants seem designed to work with 'import *'; one can then potentially use name completion for the longer names.  After 'as tk', I think "tk.Both" is worse than "'both'".  For new code with explicit imports, adding constants to the imports is extra work.  For existing code with many constants, such as help_about, adding constants to the imports is less work than converting.  So I am inclined to leave them as are until such time as we might do a search-replace throughout idlelib.

When there are a few constants, I have put them after the classes.  For this module, I would like to try a separate import rather than use '\' line continuation.

from tkinter import TOP, BOTTOM, SIDE, SUNKEN, EW, ...
---

In one file with just two constants, I initially converted to the quoted literal, but another core dev objected on the basis that the constants are checked when the function is compiled instead of when called, and that this is somehow safer.  For when issue comes up again, I am recording my current replies here.

1. The same logic would suggest that we should write, for instance, "open(filename, mode=W, encoding=ASCII, errors=STRICT)" (after appropriate imports) instead of the current "open(filename, mode='w', encoding='ascii', errors='strict')".

2. If running a file to do a test compile also does a test call of create-widget functions, then the objection does not apply.  For every IDLE file that creates tk windows, there is an htest, initiated by running the file, that creates an instance of the window.  The first purpose is to insure that widget creation calls are legitimate.  The unit tests will eventually do the same, as in 1b above.  (The second purpose, not duplicated by unittests, is to see if the result 'looks good'.)
msg294082 - (view) Author: Louie Lu (louielu) * Date: 2017-05-21 07:18
Due to the merged of #30303, text_view now have _utest attribute for unittest, upload the unittest of help_about dialog in PR 1697
msg294092 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-21 10:06
Louie, at first glance, this appears to implement the remaining changes in 1 and 2.

A possible problem is that I expect the needed 'self.' additions will conflict with the name changes on the same lines that Cheryl said she would do, about 10 hours ago.  Cheryl, if you have not yet started your patch, hold off for now, until I can properly review Louie's patch.
msg294094 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-05-21 10:27
Oh, I had started to make the changes, but that's OK.  I'm still doing discovery too.  'Fun' did mean that I'll try something.  This is exactly what I wanted to work on to dive into tkinter and idle, so I got excited.

Louie, just to coordinate, are you working on other items that Terry listed here?

Terry, should I do each item in its own PR for quick review or should I try to do many at once in a single PR?  I was doing both the name changes and docstrings (and now maybe the import change) in one PR, but maybe that's too much?
msg294095 - (view) Author: Louie Lu (louielu) * Date: 2017-05-21 10:32
Cheryl: yes, I may work on 4a/4b and msg294004 on Monday, or do you start at these tasks?
msg294100 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-21 11:01
I am about to go to bed, way late.

I would like to review and apply all the non-test changes, 4,5,6 in one PR, and add 3 since I know what I meant.  There can be multiple commits in one PR though.

Louie, from #30422 and a tracker search, you can see that there is lots to do, for more that 1 or even 2 people.  I think 4,5,6 are a good way for Cheryl to start with IDLE.  Other files need similar changes that do  not need your tkinter skills.

If you are looking for a useful challenge, there are 12 open IDLE debugger issues + some enhancements debugger ideas I have not bothered to post yet.  If that does not interest you, I can try to suggest something else after I get up again.

I expect to be away from my computer on my Monday.
msg294114 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-21 22:19
New changeset 054e09147aaa6f61aca6cd40c7bf7ce6dc54a04b by terryjreedy (mlouielu) in branch 'master':
bpo-30290: IDLE: Add more tests for help_about dialog (#1697)
https://github.com/python/cpython/commit/054e09147aaa6f61aca6cd40c7bf7ce6dc54a04b
msg294115 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-21 22:26
PR merged.  I will worry about where in the test to call AboutDialog.Ok, which has the last uncovered line  - and any other details -- later, perhaps when I merge this file into help.py.
msg294121 - (view) Author: Louie Lu (louielu) * Date: 2017-05-22 03:39
Terry: I see, I'll take some debugger stuff to try first.
Cheryl: If you got any problem on 4/5/6, feel free to ask me on IRC or here!
msg294139 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-05-22 11:24
I've made a pull request for 4, 5, and 6.  I didn't remove the tk constants as it seemed you'd like to do all the files in one project for that.

Also, in query.Query, I noticed that all the widgets were created first and then they were all added to the grid last.  I didn't move any existing lines for this, but can do that if it's preferred.  It also seemed like helper functions might make it more readable (create_py_section, create_idle_section), but maybe that's too much churn compared to what this will end up looking like.
msg294298 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-23 23:56
At first glance, 1714 looks great.  I hope to merge it tomorrow.

On the issues you raised:
1. I want to leave existing constants alone for now.
2. There are two 'create widgets' styles: create, place, create, place, ...; and create, create, ..., place, place, ... .  Both have their logic, both are used in IDLE.  The first makes sure that everything created gets placed.  The second makes it easier to arrange things in relation to each other and to rearrange.  I naively started with the first, but it seems that most experts advocate the second.
3.  If the dialog were left alone, three helper functions could be a sensible refactoring.

However, a alternate design could have links with standard blue tagging instead of buttons.  For example, the current About Firefox box has these 8 links: 'What's new', 'Mozilla', 'a global community', 'Make a donation', 'get involved', 'Licensing Information, 'End-User Rights', and 'Privacy Policy'.  This would make code much shorter and the result would not have the issues above.
msg294347 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-05-24 11:44
Thanks Terry.

I'll leave those other items alone for now.  It makes sense what you said about the 'create widget' styles.  Looking at it for the first time, I kind of liked the second version (in query) because I could more immediately understand what was happening, plus it seemed to lend itself to modularizing.  For example, have one function that has all 'create' and one that has all the 'place'.  help_about was a small dialog and there were already a lot of lines of code.  But, to your last point, blue tagging seems like it would make it all easier.

For the next step, do you have a plan in mind for what you'd like me to do next?  I know you mentioned ttk for help_about, but didn't know if you wanted me to tackle that.  Or should I PEP8 and docstring other modules such as textview.py and configdialog.py?

One question I forgot to ask before -- the `Toplevel.__init__(self, parent)` line, can this be changed to super().__init__(parent)?
msg294608 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-27 19:49
New changeset 5a346d5dbc1f0f70eca706a8ba19f7645bf17837 by terryjreedy (csabella) in branch 'master':
bpo-30290: IDLE: Refactor help_about to PEP8 names (#1714)
https://github.com/python/cpython/commit/5a346d5dbc1f0f70eca706a8ba19f7645bf17837
msg294616 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-27 22:42
While reviewing, I decided to draw a widget tree.  I believe it would have been easier if packs and grids were grouped.

I want to continue with 'IDLE: modernize help-about' and ditto for textview.  The help_about issue can begin with ttk conversion: change imports and remove an invalid option or two.  The textview issue can begin with the still missing docstrings.  I want to refactor both the way I discussed in the roadmap issue: make separate Toplevel and Frame subclasses.  The Toplevel subclass instance creates a Frame subclass instance which becomes an attribute of the toplevel.  Help.py already has this structure.
msg294620 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-05-28 01:15
Thanks Terry.  I've submitted a PR for the textview docstrings and PEP8 names and I'll work on the ttk conversion and the refactoring.
msg294625 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-28 04:39
pr 1839 is now attached to new issue #30495.  This issue is closed except for trivial bugs in the two merged PRs and backports.
msg295302 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-06 20:48
A follow-up to my brief remarks about revamping About IDLE, in msg294298:  idle-dev is subscription-required public forum for discussion of idle design issues.  It has been dormant for a year and a half, but I would like to revive it to get broader user input.   By coincidence, the first new post (held since April because the author is not subscribed) is a suggestion for AboutIDLE.  In my second response, I listed 9 possible or definite changes.  I would like that to be where we plan a future 'Modernize About IDLE' issue.
msg295615 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-10 06:53
New changeset 12cbd87ac0bb826d653040044c6b526dcdb6f6d1 by terryjreedy in branch '3.6':
[3.6] bpo-30290: IDLE - pep8 names and tests for help-about (#2070)
https://github.com/python/cpython/commit/12cbd87ac0bb826d653040044c6b526dcdb6f6d1
History
Date User Action Args
2017-06-10 06:53:21terry.reedysetmessages: + msg295615
2017-06-10 06:22:25terry.reedysetpull_requests: + pull_request2133
2017-06-10 05:31:05terry.reedysetpull_requests: + pull_request2132
2017-06-06 20:48:39terry.reedysetmessages: + msg295302
2017-05-28 05:14:47terry.reedysetpull_requests: - pull_request1922
2017-05-28 05:08:11terry.reedysetpull_requests: + pull_request1927
2017-05-28 04:39:01terry.reedysetmessages: + msg294625
2017-05-28 01:15:25csabellasetmessages: + msg294620
2017-05-28 01:12:19csabellasetpull_requests: + pull_request1922
2017-05-27 22:42:18terry.reedysetmessages: + msg294616
stage: needs patch -> commit review
2017-05-27 19:49:28terry.reedysetmessages: + msg294608
2017-05-24 11:44:20csabellasetmessages: + msg294347
2017-05-23 23:56:19terry.reedysetmessages: + msg294298
2017-05-22 11:24:47csabellasetmessages: + msg294139
2017-05-22 11:19:23csabellasetpull_requests: + pull_request1804
2017-05-22 03:39:13louielusetmessages: + msg294121
2017-05-21 22:26:37terry.reedysetmessages: + msg294115
2017-05-21 22:19:37terry.reedysetmessages: + msg294114
2017-05-21 11:01:29terry.reedysetmessages: + msg294100
2017-05-21 10:32:10louielusetmessages: + msg294095
2017-05-21 10:27:14csabellasetmessages: + msg294094
2017-05-21 10:06:15terry.reedysetmessages: + msg294092
2017-05-21 07:18:19louielusetnosy: + louielu
messages: + msg294082
2017-05-21 07:17:01louielusetpull_requests: + pull_request1790
2017-05-21 06:43:58terry.reedysetmessages: + msg294079
2017-05-21 01:37:29csabellasetmessages: + msg294064
2017-05-20 23:14:14csabellasetmessages: + msg294058
2017-05-20 05:44:00terry.reedysetmessages: + msg294004
2017-05-20 05:01:02terry.reedysetnosy: + csabella
dependencies: + IDLE: Add _utest to textview and add textview tests
messages: + msg293998
2017-05-06 05:03:19terry.reedycreate