classification
Title: configparser.InterpolationMissingOptionError is not very intuitive
Type: behavior Stage: needs patch
Components: Library (Lib) Versions: Python 3.6, Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: lukasz.langa Nosy List: Claudiu.Popa, barry, fdrake, lrowe, lukasz.langa, python-dev, rbcollins, serhiy.storchaka
Priority: normal Keywords: 3.4regression

Created on 2014-04-05 10:35 by Claudiu.Popa, last changed 2016-06-04 17:10 by lukasz.langa. This issue is now closed.

Files
File name Uploaded Description Edit
issue21159.diff lukasz.langa, 2014-09-29 05:58 review
Messages (24)
msg215589 - (view) Author: PCManticore (Claudiu.Popa) * (Python triager) Date: 2014-04-05 10:35
The error message from the title is not very intuitive from a user point of view. For instance, in the following traceback, only the first one gives a meaning to this error, through the leaked KeyError, thus the user knows that home_dir1 is invalid and missing. But the second message with "Bad value substitution" doesn't emphasize the actual problem and you have to stare at it for a couple of moments to figure out what's happening and why. Maybe changing the "Bad value substitution" with a "Missing value" would suffice. This came up in issue19546. 

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
msg215603 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-04-05 13:15
Another problem with that error message is that it seems confusing as to where the bad value *reference* came from, given that rawval doesn't seem to include any interpolation point.
msg215622 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2014-04-05 16:55
Those exception messages are really old. I agree we should fix them, we have to think about possible compatibility breakage though. I'll look into it.
msg227788 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2014-09-29 05:58
So the diff would look like this (please find it attached). It does two things:

* changes messages on InterpolationMissingOptionError and InterpolationDepthError to be more helpful to the end user
* fixes rawval to actually hold the raw value of an option and not the "rest after substitution" like before

Fred, R. David, I have two questions for you:

* the reliable method of getting the arguments of both exceptions is the .args attribute; this didn't change but the message did. It's unlikely anybody would be parsing the old message string, but even if there was I'm inclined to treat any code doing so as broken by design. Do you agree?

* without the diff, the `rawval` argument in those exceptions holds a value that exposes internal implementation and is not generally useful for user code. It wasn't exposed directly as an attribute in those exceptions (`section`, `option` and `reference` are). That being said, fixing this is a change in logic of sorts. Do you see any danger of third-party code relying on the old behaviour?

Actually, I have a hypothetical third question:

* Should I make sure that those exceptions are unpicklable by older releases of Python 3?

I'm asking because if there's no such expectation, we could add .rawval as a direct attribute to InterpolationMissingOptionError, and introduce `reference` to InterpolationDepthError (currently the exception message can say that "option O in section S contains an interpolation key that cannot be substituted" but it doesn't say which interpolation key).

Anyway, the first two questions are most important because they basically decide whether we can change the exceptions at all at this point. I'm inclined to say "yes", Python 3 did that with a number of exceptions both built-in and in the standard library.
msg227794 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-09-29 11:02
We never promise that the messages won't change (they are not part of the API), so that part isn't a problem.  We do try to be backward compatible there when it comes to args beyond the message text.  I don't think unpickleability is an issue; at least there isn't any discussion of it in PEP 3161, and I don't remember any around the implementation.

I think this is fine.  I don't think we should worry about someone who is actually using rawval/rest pulled out of the exception args.  You could put a porting note in What's New, but my guess is that the chances of anyone being inconvenienced by this are pretty near to zero.
msg237658 - (view) Author: PCManticore (Claudiu.Popa) * (Python triager) Date: 2015-03-09 15:12
I find the new error messages clear and straight to the point. It would be nice if this would get into 3.5.
msg247619 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-29 20:23
LGTM - lukasz, do you want to commit this, or would you like someone else to if you're too busy? Looks like we should patch this in 3.4/3.5./3.6 at this point.
msg248544 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-08-13 23:50
New changeset 267422f7c927 by Robert Collins in branch '3.4':
Issue #21159: Improve message in configparser.InterpolationMissingOptionError.
https://hg.python.org/cpython/rev/267422f7c927

New changeset 1a144ff2d78b by Robert Collins in branch '3.5':
Issue #21159: Improve message in configparser.InterpolationMissingOptionError.
https://hg.python.org/cpython/rev/1a144ff2d78b

New changeset fb4e67040779 by Robert Collins in branch 'default':
Issue #21159: Improve message in configparser.InterpolationMissingOptionError.
https://hg.python.org/cpython/rev/fb4e67040779
msg248545 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-13 23:53
I've applied this since it seems Lukasz was busy. Thanks for the patch Lukasz!
msg248563 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-08-14 08:08
You forgot to merge default with 3.5.
msg248564 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-14 08:10
Huh? I definitely did. I can see there's a extra head now, but I did the merge up per protocol locally.
msg248565 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-08-14 08:16
New changeset 7191910aeb45 by Robert Collins in branch 'default':
Issue #21159: Improve message in configparser.InterpolationMissingOptionError.
https://hg.python.org/cpython/rev/7191910aeb45
msg248566 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-14 08:17
anyhow fixed
msg249790 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2015-09-04 17:23
Thanks for handling this, I was indeed busy.
msg252860 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2015-10-12 15:23
Reopening because we're starting to see regressions caused by the fix for this bug, e.g.: https://bugs.launchpad.net/configglue/+bug/1504288
msg252862 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-10-12 15:29
The linked bug doesn't include any information as to what the observed failure mode(s) are.  Can you provide details, please?
msg252936 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2015-10-13 15:40
The failure is in the configglue test suite, but apparently also kazan and qutebrowser are also affected by this change.  In the Launchpad bug there's a link to a librarian build log result.

The problem is that doing the .get() requires that subclasses implement a compatible API, which wasn't required previous to this change.  But apparently adding the fallback argument to the subclass's .get() doesn't entirely fix the problem, it leads to other failures.
msg252938 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-10-13 16:14
OK, so I guess it should be backed out in 3.4.  But the since the patch does not change the signature of get, it seems like it is a legitimate change for 3.5.  It is using the public API, after all.
msg252939 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-10-13 16:16
Or are you saying the existing code is actually depending on the old value of rawval, which is what Łukasz was worried about?
msg252940 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-10-13 16:18
By the way, I won't argue a lot if you say we should go for the strict backward compatibility view even in 3.5, I'm more raising the question.
msg257662 - (view) Author: Laurence Rowe (lrowe) Date: 2016-01-06 23:45
This change is causing a problem for boto under 3.5.1 (works on 3.5.0):

TypeError: get() got an unexpected keyword argument 'raw'
> /usr/local/Cellar/python3/3.5.1/Frameworks/Python.framework/Versions/3.5/lib/python3.5/configparser.py(406)_interpolate_some()
-> rawval = parser.get(section, option, raw=True, fallback=rest)

boto bug report: https://github.com/boto/boto/issues/3433

This is because boto is subclassing ConfigParser and its get method does not include the raw argument. A quick search shows that this also affects Kazam am circus.

Circus fixed it by adding **kwargs to the method. https://github.com/circus-tent/circus/commit/d0d2ac4fd843bb9f050a8c678956fe3682371001
msg261711 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2016-03-14 02:15
I think we should close this again: if you subclass something you need to implement the full public API. raw has been in that API since 2010!.

It's a shame that folk have been implementing just-enough, rather than the actual API, but I don't see that has all that much bearing on the go-forward.

Rolling it back out of 3.4 could be done - but since the problematic subclasses are fixing themselves now 3.5 is out, that should take care of issues on 3.4 too.
msg264678 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2016-05-02 23:05
At this point I agree with Robert. It's public API that predates my changes which was inadequately mocked.

I've seen similar problems in the past when we've changed reading file objects to use plain iteration instead of the .readlines() method. I've had people complain that their "file-like object" implementation provided just .readlines() and suddenly stopped working.

I can relate to how this caused painful churn. We do try to minimize it. However, it's hard to evolve any code if we can't touch it based on fear of breaking incompletely duck typed third-party code :(

Barry, would you still like this to be reverted on Python 3.4?
msg267277 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2016-06-04 17:10
Closing since there's apparently nothing actionable at this point.
History
Date User Action Args
2016-06-04 17:10:07lukasz.langasetkeywords: - 3.5regression
status: open -> closed
resolution: fixed
messages: + msg267277
2016-05-02 23:05:37lukasz.langasetmessages: + msg264678
2016-03-14 02:15:15rbcollinssetmessages: + msg261711
2016-02-06 05:09:22ned.deilysetresolution: fixed -> (no value)
stage: resolved -> needs patch
2016-01-07 04:11:57r.david.murraysetnosy: - r.david.murray
2016-01-06 23:45:32lrowesetnosy: + lrowe
messages: + msg257662
2015-10-13 16:18:38r.david.murraysetmessages: + msg252940
2015-10-13 16:16:33r.david.murraysetmessages: + msg252939
2015-10-13 16:14:01r.david.murraysetmessages: + msg252938
2015-10-13 15:40:37barrysetmessages: + msg252936
2015-10-12 15:29:30r.david.murraysetmessages: + msg252862
2015-10-12 15:23:50barrysetkeywords: + 3.4regression, 3.5regression, - patch
status: closed -> open
messages: + msg252860
2015-10-12 15:20:17barrysetnosy: + barry
2015-09-04 17:23:16lukasz.langasetmessages: + msg249790
2015-08-14 08:17:04rbcollinssetstatus: open -> closed

messages: + msg248566
2015-08-14 08:16:27python-devsetmessages: + msg248565
2015-08-14 08:10:43rbcollinssetmessages: + msg248564
2015-08-14 08:08:03serhiy.storchakasetstatus: closed -> open
nosy: + serhiy.storchaka
messages: + msg248563

2015-08-13 23:53:31rbcollinssetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2015-08-13 23:53:08rbcollinssetmessages: + msg248545
2015-08-13 23:50:38python-devsetnosy: + python-dev
messages: + msg248544
2015-07-29 20:23:30rbcollinssetnosy: + rbcollins

messages: + msg247619
versions: + Python 3.4, Python 3.6
2015-03-09 15:12:36Claudiu.Popasetmessages: + msg237658
stage: patch review -> commit review
2014-09-29 11:02:21r.david.murraysetmessages: + msg227794
2014-09-29 05:58:56lukasz.langasetfiles: + issue21159.diff

nosy: + fdrake
messages: + msg227788

keywords: + patch
stage: needs patch -> patch review
2014-04-05 16:55:00lukasz.langasetnosy: + lukasz.langa
messages: + msg215622

assignee: lukasz.langa
stage: needs patch
2014-04-05 13:15:57r.david.murraysetnosy: + r.david.murray
messages: + msg215603
2014-04-05 10:35:12Claudiu.Popacreate