Repository navigation
bpo-30779: IDLE -- Factor ConfigChanges class from configdialog, put in config; test. #2612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
d39a5ec
32ae959
a8efe0f
1a9c0d8
3f381a3
f22fc6e
6e6f4ee
e3e5d5e
10c8681
6d2e6dd
ed50c54
b8a714f
8c708cc
67d5ea1
393f5ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,7 @@ def __init__(self, cfgFile, cfgDefaults=None): | |
| """ | ||
| cfgFile - string, fully specified configuration file name | ||
| """ | ||
| self.file = cfgFile | ||
| self.file = cfgFile # This is currently '' when testing. | ||
| ConfigParser.__init__(self, defaults=cfgDefaults, strict=False) | ||
|
|
||
| def Get(self, section, option, type=None, default=None, raw=False): | ||
|
|
@@ -73,7 +73,8 @@ def GetOptionList(self, section): | |
|
|
||
| def Load(self): | ||
| "Load the configuration file from disk." | ||
| self.read(self.file) | ||
| if self.file: | ||
| self.read(self.file) | ||
|
|
||
| class IdleUserConfParser(IdleConfParser): | ||
| """ | ||
|
|
@@ -130,21 +131,22 @@ def RemoveFile(self): | |
| def Save(self): | ||
| """Update user configuration file. | ||
|
|
||
| Remove empty sections. If resulting config isn't empty, write the file | ||
| to disk. If config is empty, remove the file from disk if it exists. | ||
| If self not empty after removing empty sections, write the file | ||
| to disk. Otherwise, remove the file from disk if it exists. | ||
|
|
||
| """ | ||
| if not self.IsEmpty(): | ||
| fname = self.file | ||
| try: | ||
| cfgFile = open(fname, 'w') | ||
| except OSError: | ||
| os.unlink(fname) | ||
| cfgFile = open(fname, 'w') | ||
| with cfgFile: | ||
| self.write(cfgFile) | ||
| else: | ||
| self.RemoveFile() | ||
| fname = self.file | ||
| if fname: | ||
| if not self.IsEmpty(): | ||
| try: | ||
| cfgFile = open(fname, 'w') | ||
| except OSError: | ||
| os.unlink(fname) | ||
| cfgFile = open(fname, 'w') | ||
| with cfgFile: | ||
| self.write(cfgFile) | ||
| else: | ||
| self.RemoveFile() | ||
|
|
||
| class IdleConf: | ||
| """Hold config parsers for all idle config files in singleton instance. | ||
|
|
@@ -766,7 +768,6 @@ def SaveUserCfgFiles(self): | |
|
|
||
| idleConf = IdleConf() | ||
|
|
||
|
|
||
| _warned = set() | ||
| def _warn(msg, *key): | ||
| key = (msg,) + key | ||
|
|
@@ -778,9 +779,99 @@ def _warn(msg, *key): | |
| _warned.add(key) | ||
|
|
||
|
|
||
| class ConfigChanges(dict): | ||
| """Manage a user's proposed configuration option changes. | ||
|
|
||
| Names used across multiple methods: | ||
| page -- one of the 4 top-level dicts representing a | ||
| .idlerc/config-x.cfg file. | ||
| config_type -- name of a page. | ||
| section -- a section within a page/file. | ||
| item -- name of a value within a section. | ||
| value -- value for the item. | ||
|
|
||
| Methods | ||
| add_item: | ||
| save_all: Save all the changes to the config file. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to add |
||
|
|
||
| reset: Clear all changes by clearing each page. | ||
| set_user_value: Set value *in idleConf* for page, section, item. | ||
| """ | ||
| def __init__(self): | ||
| "Create a page for each configuration file" | ||
| self.pages = [] # List of unhashable dicts. | ||
| for config_type in ('main', 'highlight', 'keys', 'extensions'): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question on the tuple This is in the Any benefit to making that a module constant?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left it as idleConf attribute and used the attribute here to eliminate the duplication. Good catch. |
||
| self[config_type] = {} | ||
| self.pages.append(self[config_type]) | ||
|
|
||
| def additem(self, config_type, section, item, value): | ||
| "Add item/value pair for config_type and section." | ||
| page = self[config_type] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't know if we needed to add a check for config_type to make sure it's valid before accessing it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| value = str(value) # Make sure we use a string. | ||
| if section not in page: | ||
| page[section] = {} | ||
| page[section][item] = value | ||
|
|
||
| @staticmethod | ||
| def set_value(config_type, section, item, value): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about using
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. save_option for consistency with add_option and save_all ;-). Only called in save_all,and twice in tests. |
||
| """Return True if the configuration value was added or changed. | ||
|
|
||
| Helper for save_all. | ||
| """ | ||
| if idleConf.defaultCfg[config_type].has_option(section, item): | ||
| if idleConf.defaultCfg[config_type].Get(section, item) == value: | ||
| # The setting equals a default setting, remove it from user cfg. | ||
| return idleConf.userCfg[config_type].RemoveOption(section, item) | ||
| # If we got here, set the option. | ||
| return idleConf.userCfg[config_type].SetOption(section, item, value) | ||
|
|
||
| def save_all(self): | ||
| "Save configuration changes to the user config file." | ||
|
|
||
| idleConf.userCfg['main'].Save() | ||
| for config_type in self: | ||
| cfg_type_changed = False | ||
| page = self[config_type] | ||
| for section in page: | ||
| if section == 'HelpFiles': # Remove it for replacement. | ||
| idleConf.userCfg['main'].remove_section('HelpFiles') | ||
| cfg_type_changed = True | ||
| for item, value in page[section].items(): | ||
| if self.set_value(config_type, section, item, value): | ||
| cfg_type_changed = True | ||
| if cfg_type_changed: | ||
| idleConf.userCfg[config_type].Save() | ||
| for config_type in ['keys', 'highlight']: | ||
| # Save these even if unchanged! | ||
| idleConf.userCfg[config_type].Save() | ||
| self.clear() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the result be clear after
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, it should be clear after
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By our current standard, yes. Changed. |
||
| # ConfigDialog caller must add the following call | ||
| # self.save_all_changed_extensions() # Uses a different mechanism. | ||
|
|
||
| def delete_section(self, config_type, section): | ||
| """Delete a section from self, userCfg, and file. | ||
|
|
||
| Used to delete custom themes and keysets. | ||
| """ | ||
| if section in self[config_type]: | ||
| del self[config_type][section] | ||
| configpage = idleConf.userCfg[config_type] | ||
| configpage.remove_section(section) | ||
| configpage.Save() | ||
|
|
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove one blank line
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I appreciate the review. Polishing a patch is really hard. |
||
| def clear(self): | ||
| """Clear all 4 pages. | ||
|
|
||
| Called in save_all after saving to idleConf. | ||
| XXX Mark window *title* when there are changes; unmark here. | ||
| """ | ||
| for page in self.pages: | ||
| page.clear() | ||
|
|
||
|
|
||
| # TODO Revise test output, write expanded unittest | ||
| # | ||
| if __name__ == '__main__': | ||
| def _dump(): # htest # (not really, but ignore in coverage) | ||
| from zlib import crc32 | ||
| line, crc = 0, 0 | ||
|
|
||
|
|
@@ -790,10 +881,10 @@ def sprint(obj): | |
| line += 1 | ||
| crc = crc32(txt.encode(encoding='utf-8'), crc) | ||
| print(txt) | ||
| #print('***', line, crc, '***') # uncomment for diagnosis | ||
| #print('***', line, crc, '***') # Uncomment for diagnosis. | ||
|
|
||
| def dumpCfg(cfg): | ||
| print('\n', cfg, '\n') # has variable '0xnnnnnnnn' addresses | ||
| print('\n', cfg, '\n') # Cfg has variable '0xnnnnnnnn' address. | ||
| for key in sorted(cfg.keys()): | ||
| sections = cfg[key].sections() | ||
| sprint(key) | ||
|
|
@@ -808,3 +899,9 @@ def dumpCfg(cfg): | |
| dumpCfg(idleConf.defaultCfg) | ||
| dumpCfg(idleConf.userCfg) | ||
| print('\nlines = ', line, ', crc = ', crc, sep='') | ||
|
|
||
| if __name__ == '__main__': | ||
| import unittest | ||
| unittest.main('idlelib.idle_test.test_config', | ||
| verbosity=2, exit=False) | ||
| #_dump() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer to use
add_itemin above changes? Current code is usingadditem.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing configparser parameters, changed to add_option. Will have to change all in test and configdialog and check its tests.