IDLE: configdialog -- factor out Changes class

[terryjreedy]

BPO	30779
Nosy	@terryjreedy, @csabella
PRs	bpo-30779: IDLE: Add ConfigChanges class to config.py #2610 bpo-30779: IDLE -- Factor ConfigChanges class from configdialog, put in config; test. #2612 [3.6] bpo-30779: IDLE -- Factor ConfigChanges class from configdialog… #2625 bpo-30779: News #2627 [3.6] bpo-30779: News (GH-2627) #2630 bpo-30779: IDLE: fix delete_section calls in configdialog #2667 [3.6] bpo-30779: IDLE: fix changes.delete_section calls in configdial… #2674
Files	changes_class.py changes_class_2.py configdialog_tests_v1.py changes_class_v3.py changes_class_v4.py: Add to config.py

^{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 = <Date 2017-07-08.00:03:16.067>
created_at = <Date 2017-06-26.22:34:16.889>
labels = ['expert-IDLE', 'type-feature', '3.7']
title = 'IDLE: configdialog -- factor out Changes class'
updated_at = <Date 2017-07-11.23:50:12.773>
user = 'https://github.com/terryjreedy'

bugs.python.org fields:

activity = <Date 2017-07-11.23:50:12.773>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2017-07-08.00:03:16.067>
closer = 'terry.reedy'
components = ['IDLE']
creation = <Date 2017-06-26.22:34:16.889>
creator = 'terry.reedy'
dependencies = []
files = ['46980', '46981', '46984', '46985', '46991']
hgrepos = []
issue_num = 30779
keywords = []
message_count = 23.0
messages = ['296962', '297203', '297249', '297303', '297329', '297343', '297462', '297804', '297807', '297837', '297850', '297857', '297882', '297886', '297908', '297909', '297913', '297919', '297922', '298155', '298187', '298188', '298191']
nosy_count = 2.0
nosy_names = ['terry.reedy', 'cheryl.sabella']
pr_nums = ['2610', '2612', '2625', '2627', '2630', '2667', '2674']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue30779'
versions = ['Python 3.6', 'Python 3.7']

[terryjreedy]

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.

[csabella]

If possible, I'd like to work on this.

[terryjreedy]

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.

[terryjreedy]

High, because other issues depend on this.

[csabella]

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!

[terryjreedy]

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.

[csabella]

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 Rename README to README.rst and enhance formatting #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!

[terryjreedy]

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.

[terryjreedy]

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 bpo-30780.

[terryjreedy]

I am working on this now.

[csabella]

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!