classification
Title: IDLE: configdialog -- factor out Changes class
Type: enhancement Stage: resolved
Components: IDLE Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: cheryl.sabella, terry.reedy
Priority: high Keywords:

Created on 2017-06-26 22:34 by terry.reedy, last changed 2017-07-11 23:50 by terry.reedy. This issue is now closed.

Files
File name Uploaded Description Edit
changes_class.py cheryl.sabella, 2017-06-29 23:59
changes_class_2.py terry.reedy, 2017-06-30 07:41
configdialog_tests_v1.py cheryl.sabella, 2017-07-01 00:51
changes_class_v3.py cheryl.sabella, 2017-07-01 00:52
changes_class_v4.py terry.reedy, 2017-07-06 05:23 Add to config.py
Pull Requests
URL Status Linked Edit
PR 2610 closed cheryl.sabella, 2017-07-06 22:45
PR 2612 merged terry.reedy, 2017-07-07 02:27
PR 2625 merged terry.reedy, 2017-07-07 20:07
PR 2627 merged terry.reedy, 2017-07-08 00:48
PR 2630 merged terry.reedy, 2017-07-08 02:13
PR 2667 merged cheryl.sabella, 2017-07-11 12:55
PR 2674 merged terry.reedy, 2017-07-11 23:20
Messages (23)
msg296962 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-26 22:34
configdialog.ConfigDialog mixes together two things: a user interface for  changing options and a mechanism for storing proposed changes and applying them to the configuration dictionaries managed by config.  The change mechanism is based on a 3-level dict that maps user config file, config file section, and section entry to a new values.  Most GUI event handlers insert entries into the dict.  Cancel clears the dict. Apply and call a method to apply changes (and then clear them).

This issue will factor out the changes structure and all methods that touch it into a new Changes class.  ConfigDialog needs to be able to call methods to add changes, clear changes, apply changes, and maybe something else.  GUI tests need to be able to query currently stored changes.

Tests of Changes, which are part of this issue and should be 'complete', will accept examples changes from the test functions and send changes to either configparser mocks or even the actual configparser structures, but not to disk.

A complete test of the GUI (not part of this issue) will simulate user interaction with every widget and then query Changes() to see that the proper changes orders have been recorded.
msg297203 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-06-28 17:42
If possible, I'd like to work on this.
msg297249 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-29 03:23
Let's do it 'right'.

Step 1: Write class design in the form of a class with methods with APIs and docstrings as the bodies.

class Changes:
   """Manage a user's proposed configuation option changes.

   Changes are either cancelled or applied in a single transaction.
   Methods used by ConfigDialog:
   meth_a does xa
   Methods used only for testing (if any)
   meth_t does xt for testing
   Internal methods not called externally:
   meth_b does xb.
   """

   def __init__(self, ???):  # new
   """Create configuration option change manager.

   arg: bool, do we like George Boole?
   """

Temporarily comment methods as new, old, renamed from whatever.

Step 2. Test design by writing a test class with at least a few test methods.  If methods are not easy to write, perhaps we should change design.

class ChangesTest(unittest.TestCase):
# no gui needed.    

Step 3. Write simple, clear bodies that make tests pass.

However much you submit, I will review in that order: design, tests, implementation.
msg297303 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-29 19:57
High, because other issues depend on this.
msg297329 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-06-29 23:59
Here's the beginning design.  Since it's a high priority, I didn't want to wait too long to show you something.  As you can see, I still had a lot of questions and things I wasn't sure about.

I have some tests, but wanted to have it more complete before submitting it.

When you said 'submit', I didn't know if you wanted a real PR at each step or just files here.  If you'd prefer a PR so you could modify it, let me know and I'll prepare that tomorrow.

Thanks!
msg297343 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-30 07:41
The question about reusing ConfigParser (or IdleConfigParser) was a good one.  I spent at least an hour looking again at the code and configparser doc.

Observations:
1. Dropping 2.7 support allows us to use mapping protocol access:
https://docs.python.org/3/library/configparser.html#mapping-protocol-access
2. 'ConfigParser is a misleading name for what is essentially a dict of dict that may never use its parser.  Sections are parts of a page of text.
3. idleConf manages 8 pages, organized in 2 groups (default and user) of 4.  Each group of 4 represents a 'set of pages' stored in a directory.  There is no class for this concept, but some idleConf methods should be methods of the missing class.
4. The 8 pages can also be seen as 4 pairs.  Some idleConf methods, especially those getting values, operate on 1 of the 4 pairs.  Maybe there should also be a Pair class.

Possibly patching config based on these thoughts is for the future, but I decided that Changes needs a Page class.  Each tab works with one page, and should call Page methods.  Changes is initialized by ConfigDialog proper and its methods are called by its buttons, not by tabs.  I am not a fanatic follower of the 'law of demeter' but the idea of direct access when possible is correct. 

Next steps: Write class PageTest.  Don't overdo it.  The code is simple and just needs inputs testing each branch.  (I know, I violated write the tests first. Hmmm.  Seeing the code simplify so nicely convinced me that the design was good ;-) Write ChangesTest and pull in and adapt the existing code for the Changes methods to make them work.

For the moment, I say stick with uploads, perhaps until we are ready to remove old code and integrate this into configdialog.py.  At some point we might learn how to collaborate via git branches without making PRs.  In the meanwhile, right click, save, git apply are easy enough for me. 

You asked: Move all IdleConf.userCFg and IdleConf.defaultCfg access to Changes to separate that completely from GUI?

I don't think this would work because the data structures are different.  It also seems conceptually wrong.  Changes embodies the concept 'Pending changes', which constitute a potential transaction.

If there are repeated pattern of interaction with idleConf that have not already been captured in idleConf methods, we could factor out another class (Config? Options?) in another issue.  Key questions: would this make testing easier?  would this significantly improve ConfigDialog code?

I  will try to look at the extension stuff tomorrow or soon thereafter.
msg297462 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-07-01 01:17
Thanks for pointing out mapping protocol access.  That's what I was (poorly) trying to describe in the __getitem__ docstring in v1.  It's a really concept.

I worked on the tests.  I added some more code to the classes, so I uploaded that too (sorry, but I needed help understanding the functions).

Testing your Page class seemed pretty easy, so hopefully I did it right.  I have added more methods to the class though, which I didn't add tests for yet, in case it's not the right direction.

I changed the existing test to use the Changes class.  I believe I did what you intended, but I didn't expand it to keys and highlights just in case.

Now, my difficulty.  I had trouble trying to figure out the test for 'save_as', so I wanted to show you what I had so far.  You don't need to fill it out; I just needed to see if I was on the right track.  Writing a test without code was difficult since it involved other classes.  So, I copied the existing code into the class to see how it would fit.  Observations:
1.  The `testcfg` was a IdleUserConfParser dictionary.  I made a dummy config parser to override Save instead of mocking it.  I think you prefer this way of doing that.
2.  Page needed to know it's 'config_type' so that it (or Changes) could reach into idleConf.  self.main on its own didn't know which part of the  config files to update.
3.  Following #2, I created a 'save' module within Page.  My options were idleConf.userCfg[page.name] (in Changes) or idleConf.userCfg[self.name] (in Page).  Based on your LoD comment, I thought Page should know about that and not Changes.
4.  Split out `save_as` into another method called `changed`, also in Page.  I thought it made sense for a Page to say if it's changed.
5.  set_user_value is the same as before, although I changed the docstring and it's in Page for the page.name vs self.name reason.  However, this still looks really clunky to me.

Questions:
1.  Any interest on making Page inherit from defaultdict to simplify additem?
2.  I'm not sure about the name for Page, but I haven't come up with an alternate.  My reason is that in configdialog, all the create functions are create_page_* and the names on the tabs don't match the config (eg, there's a General tab stored in main).  Unless you made that connection on purpose?  I was thinking of `page` more as a GUI item, maybe because of terms like webpage.  Maybe the solution is to make the functions create_tab_*?

I still haven't looked at extensions.  I should have more time tomorrow.

One note, I found a typo in the current test file.  In tearDownModule, it sets idleConf.userCfg to testcfg, but I think the intent was to set it back to the saved value, which is userCfg.

Thanks!
msg297804 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-06 05:23
To get moving and unblock other issues, I decided to start with the simplest not-bad change that will work.

The factored out code belongs in config.py. This will separate integration with ConfigDialog into 2 steps.
1. Add code and tests to config and test_config, leaving configdialog untouched.
2. Modify configdialog and tests to import and use the new classes, removing the refactored classes.

We should start with ConfigChanges as a subclass of dict, with added methods as unchanged as possible.  No Page class, but the page idea appears, and will later be applied elsewhere.  No apply method, as the code only accesses changes but does not modify them.  No save_new_config, as that code bypasses changes completely. No set_user_value, as that only access idleConf.  I guessed at what delete_section should look like as I don't know where the original code is.

Attached is my untested ConfigChanges.

With step 1 done, I believe step 2 would consist of:

A. add ConfigChanges to the config import.

B. make 'changes' a global (like idleConf), after imports, with
changes = ConfigChanges()
[This could go in config, but CD is its only user.]
Remove self.reset_changed_items in CD.__init__.

C. replace names throughout file
/self.changed_items/changes/
/self.reset_changed_items/changes.clear/
/self.add_changed_items/changes.add_item/ 
/self.save_all_changed_configs/changes.save_all/

C. Removed renamed methods.  (Could do before B.)
D. Replace code moved to .delete_section with a call thereto.

E. Change test_configdialog
If you wrote tests for methods not included in ConfigChanges, they could be added here anyway.

PR2 could be prepared before PR1 is merged, but must not be applied before PR1 is merged and backported, and has to be consistent with final applied version of PR1.
msg297807 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-06 05:53
Comments on difficulties and questions.

D1. I will look at test changes where there is a PR to run.  I wrote what you changed, but might not have considered what you did.
D2. Moot for the moment.
D3. Ditto.
D4. If not needed, skip for now.
D5. I will be more comfortable changing and presumably improving working code when we have good tests.  GetDefaultItems (needs renaming) could save references to page and section dicts.

We are in a bit of a chicken and egg situation when we need better tests to improve code and better code to write better tests.  After steps 1 and 2, I want to work on tests.  Then we can work on more code changes.

Q1. dict.get is a way to avoid the existence check, and I am thinking of trying it out when tests are in place.  It may not have existed when this code was written.  I don't know what else DefaultDict adds.  
Q2. I was aware of the connection to tab pages, but it is not exact.  I decided on 'config_type' for the string argument and local name page for the corresponding dict.

Note: Yes, a bug.  I save usercfg so I could restore it.  The fix should be a PR for #30780.
msg297837 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-06 19:32
I am working on this now.
msg297850 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-07-06 23:05
I've made the first pull request for the changes to config.py.  I have to apologize because I really couldn't figure out how to do 'save_all' without including 'set_user_value' since 'set_user_value' updates userCfg with the values from ConfigChanges.

Also, in `__init__`, you had self.pages, but *self* itself is the same thing, i.e. self['main'], etc is created in the for-loop, so I couldn't figure out the difference between self and self.pages, except in the context of the Page class.

I kept the code as close to the original as possible.  One side effect is that sometimes we have `self[page]` and sometimes `self[config_type]`.

I also commented out the call to `self.save_all_changed_extensions` in `save_all`.  In configdialog, `save_all_changed_extensions` iterates over what would be ConfigChanges['extensions'], with the section = ext_name and item = opt.  It calls `set_extension_value` with each ext_name/opt, but the difference between this and the others is that opt (item) itself is a dictionary with 'name', 'default', and 'var' keys, whereas 'main', 'keys', and 'highlight' have items with one value (at least in set_user_value).

So, if `set_user_value` stays in ConfigChanges, I believe `set_extension_value` and `save_all_changed_extensions` can be copied there as intact also.  Neither one uses anything from configdialog directly, just things from ConfigChanges and idleConf.

Thanks!
msg297857 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-07 03:45
Whoops, we seems to have partly overlapped (and made some of the same changes).  I plan to push my PR sometime tomorrow (it also changes configdialog and revised tests) after looking as yours.  Then move on to adding config tests, #30780.

The tests for config need the real config parsers, except for save.  But to fully test config, a mock save is needed.  I opened #30868 for this.

set_value (set_user_value) accesses neither configdialog nor changes.  So it is a function rather than a method, and I could have put it in config as such.  I added it as a staticmethod just above save_all instead because it is only used by save_all.

Any other of the now 74 methods of ConfigDialog that are not really methods thereof (don't use self) should also be removed and made module functions, in either module, or possibly changes methods.  I just want them tested first, if possible.

I *would* like to put all the extension changes in changes instead of some in a separate structure.  I believe the 'hard' part is that key changes have to be handled separately from the non-extension keys.  The code was contributed by someone else and once I determined that it worked by manual testing, I left it alone. Testing and moving and possibly rewriting the extension stuff would be a coherent issue and PR.
msg297882 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-07-07 12:19
Sorry about that.  When I read msg297804, I thought your intention was for me to work on the PRs.  I didn't see your other note until now.  It will be good for me to learn from your changes.

As far as mocking Save, I sort of did that (very basically) in my PR by using Func().  It wasn't covering everything because I only added it for 'main'.  Do you think I should continue with that in #30868 or did you want to do it a different way?
msg297886 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-07-07 14:52
Terry,

Any interest on using gitter or slack (or something like that) while trying to coordinate work between you, Louie, me, and others?
msg297908 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-07 20:00
New changeset 349abd9e37dfdc077bc21f19e6ed2292c767f0e8 by terryjreedy in branch 'master':
bpo-30779: IDLE -- Factor ConfigChanges class from configdialog, put in config; test. (#2612)
https://github.com/python/cpython/commit/349abd9e37dfdc077bc21f19e6ed2292c767f0e8
msg297909 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-07 20:37
New changeset edc034221f878b437748cef0f05c782d8190499d by terryjreedy in branch '3.6':
[3.6] bpo-30779: IDLE -- Factor ConfigChanges class from configdialog, put in config; test. (GH-2612) (#2625)
https://github.com/python/cpython/commit/edc034221f878b437748cef0f05c782d8190499d
msg297913 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-07 21:39
Cheryl, thank you for reviewing.  I am answering the question about #30868 on that issue.

I did not want to immediately test user parser Save() calls in the .save_all test for two reasons.

1. I want to first reconsider the general question what to do about Save in the new issue.

2. The only specification for .save_all calls to .Save is the code itself, and I am not convinced it is correct.  I want to review before semi-freezing the code by testing that it does what it does.

Even if it is correct, it will will be tricky to test all paths.  For instance, save_all unconditionally call usermain.Save before looking at changes.  Then, if changes has changes for usermain, usermain.Save is called again.  To test that there are two saves, Func could easily be changed to increment .called instead of setting it to 1.

My guess for the double save is that ConfigDialog *might* write some changes directly to userCfg, so the unconditional save makes sure they are written.  But why bypass?  If the dialog is cancelled, the 'unconditional' save will not happen, and any directly written changes might affect the current session, but might never be saved to disk.  That would seem like a bug.

I have similar questions about the unconditional save of highlights and keys.  If all three files are always overwritten, why not push any changes to userCfg and then write all three without bothering about keeping check if there are additional changes?

I am leaving this until I/we have reviewed configdialog while writing tests.
msg297919 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-08 00:49
New changeset 24f2e19d68cc6ca563d2be5944d11d5f55a46918 by terryjreedy in branch 'master':
bpo-30779: News (#2627)
https://github.com/python/cpython/commit/24f2e19d68cc6ca563d2be5944d11d5f55a46918
msg297922 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-08 02:26
New changeset 9d8abf31c430dd83d073660cc92f1fe4ca6f2cd4 by terryjreedy in branch '3.6':
[3.6] bpo-30779: News (GH-2627) (#2630)
https://github.com/python/cpython/commit/9d8abf31c430dd83d073660cc92f1fe4ca6f2cd4
msg298155 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-07-11 12:58
I had an error when trying to delete a custom theme.  Sorry for not catching this before.
msg298187 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-11 22:55
Eventually, we will have tests that would have caught this.  I took the opportunity to improve the test for the call.
msg298188 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-11 23:09
New changeset 6d13b22e3ab262c6b1f068259aebd705e7da316c by terryjreedy (csabella) in branch 'master':
bpo-30779: IDLE: fix changes.delete_section calls in configdialog (#2667)
https://github.com/python/cpython/commit/6d13b22e3ab262c6b1f068259aebd705e7da316c
msg298191 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-11 23:50
New changeset c0179483f13be81910ed73889dcad92528e20ef2 by terryjreedy in branch '3.6':
[3.6] bpo-30779: IDLE: fix changes.delete_section calls in configdialog (GH-2667) (#2674)
https://github.com/python/cpython/commit/c0179483f13be81910ed73889dcad92528e20ef2
History
Date User Action Args
2017-07-11 23:50:12terry.reedysetmessages: + msg298191
2017-07-11 23:20:39terry.reedysetpull_requests: + pull_request2742
2017-07-11 23:09:46terry.reedysetmessages: + msg298188
2017-07-11 22:55:07terry.reedysetmessages: + msg298187
2017-07-11 12:58:03cheryl.sabellasetmessages: + msg298155
2017-07-11 12:55:59cheryl.sabellasetpull_requests: + pull_request2733
2017-07-08 02:26:55terry.reedysetmessages: + msg297922
2017-07-08 02:13:48terry.reedysetpull_requests: + pull_request2695
2017-07-08 00:49:40terry.reedysetmessages: + msg297919
2017-07-08 00:48:52terry.reedysetpull_requests: + pull_request2693
2017-07-08 00:03:16terry.reedysetstatus: open -> closed
resolution: fixed
stage: test needed -> resolved
2017-07-07 21:39:06terry.reedysetmessages: + msg297913
2017-07-07 20:37:41terry.reedysetmessages: + msg297909
2017-07-07 20:07:28terry.reedysetpull_requests: + pull_request2691
2017-07-07 20:00:59terry.reedysetmessages: + msg297908
2017-07-07 14:52:49cheryl.sabellasetmessages: + msg297886
2017-07-07 12:19:23cheryl.sabellasetmessages: + msg297882
2017-07-07 03:45:44terry.reedysetmessages: + msg297857
2017-07-07 02:27:07terry.reedysetpull_requests: + pull_request2677
2017-07-06 23:05:12cheryl.sabellasetmessages: + msg297850
2017-07-06 22:45:18cheryl.sabellasetpull_requests: + pull_request2675
2017-07-06 19:32:11terry.reedysetmessages: + msg297837
2017-07-06 05:53:14terry.reedysetmessages: + msg297807
2017-07-06 05:23:39terry.reedysetfiles: + changes_class_v4.py

messages: + msg297804
2017-07-01 01:18:00cheryl.sabellasetmessages: + msg297462
2017-07-01 00:52:10cheryl.sabellasetfiles: + changes_class_v3.py
2017-07-01 00:51:55cheryl.sabellasetfiles: + configdialog_tests_v1.py
2017-06-30 22:38:18terry.reedylinkissue27099 dependencies
2017-06-30 07:41:22terry.reedysetfiles: + changes_class_2.py

messages: + msg297343
2017-06-29 23:59:42cheryl.sabellasetfiles: + changes_class.py

messages: + msg297329
2017-06-29 19:57:53terry.reedysetpriority: normal -> high

messages: + msg297303
2017-06-29 03:23:08terry.reedysetmessages: + msg297249
2017-06-28 17:42:06cheryl.sabellasetnosy: + cheryl.sabella
messages: + msg297203
2017-06-27 00:46:41terry.reedylinkissue30728 dependencies
2017-06-26 22:40:08terry.reedysetassignee: terry.reedy
components: + IDLE
2017-06-26 22:39:41terry.reedylinkissue30780 dependencies
2017-06-26 22:34:16terry.reedycreate