configparser leaks implementation detail

[PCManticore]

BPO	19546
Nosy	@bitdancer, @PCManticore, @ambv
Files	configparser.patch issue19546.patch: Remove trailing whitespace. issue19546_1.patch: Small style fixes.

^{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/ambv'
closed_at = <Date 2014-09-04.08:43:53.703>
created_at = <Date 2013-11-10.17:16:52.435>
labels = ['type-bug', 'library']
title = 'configparser leaks implementation detail'
updated_at = <Date 2014-09-04.08:45:13.784>
user = 'https://github.com/PCManticore'

bugs.python.org fields:

activity = <Date 2014-09-04.08:45:13.784>
actor = 'Claudiu.Popa'
assignee = 'lukasz.langa'
closed = True
closed_date = <Date 2014-09-04.08:43:53.703>
closer = 'lukasz.langa'
components = ['Library (Lib)']
creation = <Date 2013-11-10.17:16:52.435>
creator = 'Claudiu.Popa'
dependencies = []
files = ['32564', '34837', '35791']
hgrepos = []
issue_num = 19546
keywords = ['patch']
message_count = 14.0
messages = ['202538', '202552', '215334', '215336', '215337', '215590', '215591', '215604', '215621', '216159', '221674', '226350', '226351', '226352']
nosy_count = 4.0
nosy_names = ['r.david.murray', 'Claudiu.Popa', 'lukasz.langa', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue19546'
versions = ['Python 3.4', 'Python 3.5']

[PCManticore]

Various exceptions raised by configparser module leaks implementation detail, by chaining KeyErrors, as seen below:

Python 3.4.0a4+ (default:0aa2aedc6a21+, Nov  5 2013, 17:10:42)
[GCC 4.2.1 20070831 patched [FreeBSD]] on freebsd8
Type "help", "copyright", "credits" or "license" for more information.
>>> import configparser
>>> parser = configparser.ConfigParser()
>>> parser.remove_option('Section1', 'an_int')
Traceback (most recent call last):
  File "/tank/libs/cpython/Lib/configparser.py", line 935, in remove_option
    sectdict = self._sections[section]
KeyError: 'Section1'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/tank/libs/cpython/Lib/configparser.py", line 937, in remove_option
    raise NoSectionError(section)
configparser.NoSectionError: No section: 'Section1'
>>>

There are multiple places where this happens: using basic and extended interpolation, using .options to retrieve non-existent options, using .get or .remove_options for non-existent options/sections. The attached patch tries to fixes all those issues by suppressing the initial exception.

[bitdancer]

I'd vote -1 on this one. The extra context in this case is not confusing, and might be helpful to someone.

[PCManticore]

But the last traceback conveys enough information, the user can see immediately that the given section does not exist.
My problem with the current behaviour is that the first error distracts the user, while the actual problem is the second traceback.
But I have another example which might contradict my proposal. In the example in the second traceback, it's not clear what Bad value substitution stands for and the first traceback adds the relevant context (the fact that key is missing). But I guess this stands for another issue, for enhancing the message for InterpolationMissingOptionError to transmit the fact that a given option is missing.

Traceback (most recent call last):
  File "C:\Python34\lib\configparser.py", line 410, in _interpolate_some
    v = map[var]
  File "C:\Python34\lib\collections\__init__.py", line 789, in __getitem__
    return self.__missing__(key)            # support subclasses that define __missing__
  File "C:\Python34\lib\collections\__init__.py", line 781, in __missing__
    raise KeyError(key)
KeyError: 'home_dir1'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Python34\lib\configparser.py", line 1204, in __getitem__
    return self._parser.get(self._name, key)
  File "C:\Python34\lib\configparser.py", line 773, in get
    d)
  File "C:\Python34\lib\configparser.py", line 374, in before_get
    self._interpolate_some(parser, option, L, value, section, defaults, 1)
  File "C:\Python34\lib\configparser.py", line 413, in _interpolate_some
    option, section, rest, var)
configparser.InterpolationMissingOptionError: Bad value substitution:
        section: [Paths]
        option : my_dir
        key    : home_dir1
        rawval : /lumberjack

[bitdancer]

Why? Just let the context convey the information. It's not like we are building a UI here, this is for the programmer.

[bitdancer]

Although I will grant you that I have to guess at what the bad value substitution error message is trying to tell me, so that error message could use some improvement.

[PCManticore]

I've created a new issue for the InterpolationMissingOptionError message, bpo-21159. This issue can be closed.

[PCManticore]

Ups, sorry for the change of resolution.

[bitdancer]

I've been thinking about this more, and I think I will revise my opinion. I haven't been able to think of a place where knowing that the key is missing in self._sections would in fact be helpful to a programmer using the module. So I'm OK with this being an example of a place where the new error fully encapsulates the information from the context and therefore the context can be suppressed.

[ambv]

FWIW I agree with Claudiu that the internal exceptions are an implementation detail. If we ever made, say, a SQLite-based or memcache-based configparser, those would be different, but the external API would stay the same.

Will fix.

[PCManticore]

If there is anything left to do for this patch, please tell me.

[PCManticore]

Łukasz, do you have some time to take a look at this patch?

[python-dev]

New changeset 2b14665b7bce by Łukasz Langa in branch 'default':
Fix bpo-19546: onfigparser exceptions expose implementation details. Patch by Claudiu Popa.
http://hg.python.org/cpython/rev/2b14665b7bce

New changeset 554ead559f24 by Łukasz Langa in branch 'default':
Merge fix for bpo-19546: configparser exceptions leak implementation details
http://hg.python.org/cpython/rev/554ead559f24