bpo-30779: IDLE -- Factor ConfigChanges class from configdialog, put in config; test. by terryjreedy · Pull Request #2612 · python/cpython

terryjreedy · 2017-07-07T02:27:05Z

No description provided.

mention-bot · 2017-07-07T02:27:09Z

@terryjreedy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @csabella, @asvetlov and @kbkaiser to be potential reviewers.

mlouielu

Some method name need to consider to changed.

mlouielu · 2017-07-07T03:34:12Z

+        configpage.remove_section(section)
+        configpage.Save()
+
+


Please remove one blank line

Done. I appreciate the review. Polishing a patch is really hard.

mlouielu · 2017-07-07T03:46:29Z

+        value -- value for the item.
+
+    Methods
+        add_item:


Would you prefer to use add_item in above changes? Current code is using additem.

After reviewing configparser parameters, changed to add_option. Will have to change all in test and configdialog and check its tests.

mlouielu · 2017-07-07T03:47:10Z

+
+    Methods
+        add_item:
+        save_all: Save all the changes to the config file.


Need to add set_value document (will dev use this method alone?)

mlouielu · 2017-07-07T03:50:14Z

+        page[section][item] = value
+
+    @staticmethod
+    def set_value(config_type, section, item, value):


How about using save_item for consistency?

save_option for consistency with add_option and save_all ;-). Only called in save_all,and twice in tests.

mlouielu · 2017-07-07T05:11:38Z

+        for config_type in ['keys', 'highlight']:
+            # Save these even if unchanged!
+            idleConf.userCfg[config_type].Save()
+        self.clear()


Should the result be clear after save_all?
If should, then the docstring need to note it.

~~But I think it should not clear after save_all, this should let dev to do when needed~~

Ah, it should be clear after save_all, that mean the changed has been apply. Maybe this side-affect need to be documented.

By our current standard, yes. Changed.

mlouielu · 2017-07-07T05:18:57Z

 def tearDownModule():
    global root, configure
-    idleConf.userCfg = testcfg
+    idleConf.userCfg = usercfg


I think you should rebase/merge master branch since this changed has been merged into via PR #2606.

I though maybe I branched after fetching that. Anyway, I fetched again, pushed master to origin, and
ran <git merge origin/master>, from the devguide, which has worked before. and pushed.
... Above -+ is gone from new diff.

csabella · 2017-07-07T12:41:50Z

+    def __init__(self):
+        "Create a page for each configuration file"
+        self.pages = []  # List of unhashable dicts.
+        for config_type in ('main', 'highlight', 'keys', 'extensions'):


Question on the tuple ('main', 'highlight', 'keys', 'extensions')

This is in the IdleConf.__init__:
self.config_types = ('main', 'extensions', 'highlight', 'keys')

Any benefit to making that a module constant?

I left it as idleConf attribute and used the attribute here to eliminate the duplication. Good catch.

csabella · 2017-07-07T13:21:25Z

-        self.changed_items[typ][section][item] = value
+        changes.add_option('main', 'EditorWindow', 'encoding', value)

    def GetDefaultItems(self):


I can't find GetDefaultItems used anywhere. Should it be removed?

I grepped the file for 'default' to make sure you had not changed the name elsewhere to somethingdefaultsomething. 55 hit, not orphan function call. My guess is that someone wrote this, possibly for the check in save_option, and someone (same or other) decided to stick with idleConf instead of the skeletal duplicate, but did not remove.

I removed it in a separate commit.

csabella · 2017-07-07T13:40:33Z

+
+    def add_option(self, config_type, section, item, value):
+        "Add item/value pair for config_type and section."
+        page = self[config_type]


I didn't know if we needed to add a check for config_type to make sure it's valid before accessing it.

What would add_option do if it did? It expects new sections. New config-types are a bug. A test of the user, config dialog, should fail. General principle: only check first or catch after if one has an alternative action in mind that is better than letting an exception bubble up.

terryjreedy · 2017-07-07T18:41:40Z

+
+    def test_clear(self):
+        changes = self.load()
+        changes.clear()


Test_add_option verifies that load loads something. I added a comment to def load...

…, put in config; test. (pythonGH-2612) * In config, put dump test code in a function; run it and unittest in 'if __name__ == '__main__'. * Add class config.ConfigChanges based on changes_class_v4.py on bpo issue. * Add class test_config.ChangesTest, partly based on configdialog_tests_v1.py on bpo issue. * Revise configdialog to use ConfigChanges, mostly as specified in tracker msg297804. * Revise test_configdialog to match configdialog changes. All tests pass in both files. * Remove configdialog functions unused or moved to ConfigChanges. Cheryl Sabella contributed parts of the patch. (cherry picked from commit 349abd9)

…, put in config; test. (GH-2612) (#2625) * In config, put dump test code in a function; run it and unittest in 'if __name__ == '__main__'. * Add class config.ConfigChanges based on changes_class_v4.py on bpo issue. * Add class test_config.ChangesTest, partly based on configdialog_tests_v1.py on bpo issue. * Revise configdialog to use ConfigChanges, mostly as specified in tracker msg297804. * Revise test_configdialog to match configdialog changes. All tests pass in both files. * Remove configdialog functions unused or moved to ConfigChanges. Cheryl Sabella contributed parts of the patch. (cherry picked from commit 349abd9)

terryjreedy added 8 commits July 6, 2017 15:58

Make dump a function; run unittest in 'if __name__:'.

d39a5ec

Add class ConfigChanges (changes_class_v4.py on issue).

32ae959

Make sure that filesystem not touched with empty file name.

a8efe0f

Test ConfigChanges and modify as needed.

1a9c0d8

Tweaks to config and test while editing configdialog.

3f381a3

Initial revisions of configdialog as in tracker msg297804.

f22fc6e

Revise configdialog tests to match other changes. All pass.

6e6f4ee

Remove configdialog functions reproduced in new class.

e3e5d5e

the-knights-who-say-ni added the CLA signed label Jul 7, 2017

Whitespace

10c8681

mlouielu suggested changes Jul 7, 2017

View reviewed changes

Changes inspired by Louie's comments.

6d2e6dd

mlouielu reviewed Jul 7, 2017

View reviewed changes

terryjreedy added 3 commits July 7, 2017 02:32

Merge remote-tracking branch 'origin/master' into cf

ed50c54

Merge remote-tracking branch 'origin/master' into cf

b8a714f

Improve save_all docstring as per Louie's comment.

8c708cc

csabella reviewed Jul 7, 2017

View reviewed changes

terryjreedy commented Jul 7, 2017

View reviewed changes

terryjreedy added 2 commits July 7, 2017 15:04

Config changes responding to Cheryl's comments.

67d5ea1

Remove unused configdialog.GetDefaultItems.

393f5ec

python deleted a comment from csabella Jul 7, 2017

terryjreedy merged commit 349abd9 into python:master Jul 7, 2017

terryjreedy deleted the cf branch July 7, 2017 20:01

terryjreedy mentioned this pull request Jul 7, 2017

[3.6] bpo-30779: IDLE -- Factor ConfigChanges class from configdialog… #2625

Merged

Uh oh!

Conversation

terryjreedy commented Jul 7, 2017

Uh oh!

mention-bot commented Jul 7, 2017

Uh oh!

mlouielu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlouielu Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlouielu Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

terryjreedy Jul 7, 2017 •

edited

Loading

mlouielu Jul 7, 2017 •

edited

Loading

terryjreedy Jul 7, 2017 •

edited

Loading

mlouielu Jul 7, 2017 •

edited

Loading

terryjreedy Jul 7, 2017 •

edited

Loading

terryjreedy Jul 7, 2017 •

edited

Loading