classification
Title: configparser.ConfigParser's getint, getboolean and getfloat don't accept `vars`
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: lukasz.langa Nosy List: fdrake, georg.brandl, lukasz.langa, michael.foord, pitrou
Priority: normal Keywords: easy, needs review, patch

Created on 2010-07-29 18:32 by lukasz.langa, last changed 2010-11-12 00:59 by lukasz.langa. This issue is now closed.

Files
File name Uploaded Description Edit
issue9421.diff lukasz.langa, 2010-09-02 22:20 Patch that introduces the vars= and default= arguments for every getter in every parser. With unit tests and updated documentation. review
Messages (15)
msg111986 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2010-07-29 18:32
ConfigParser (and in effect SafeConfigParser) accept `vars` in the `get()` method so one can override values from the section, merge data from different sources or simply state some required keys for interpolation.

`getint()`, `getfloat()` and `getboolean()` don't accept this argument and this is the reason for this issue: for all getters to present a consistent interface.

There is a small design question here however. We have an inheritance model in configparser that goes like this:

  RawConfigParser -> ConfigParser -> SafeConfigParser

ConfigParser redefines `get()` for the purposes of interpolation but doesn't touch the other getters. It's good because in terms of implementation, they are just `type()` decoration over `get()` where type is `int`, `float` or `bool`.

The obvious solution would be to add **kwargs to these getters so that regardless of the implementation of the actual `get()`, all possible values are being passed to it. We can then comment in the docstring that for possible values of **kwargs, the user should check the interface of `get()`. Is that good enough?

Other ideas?

PS. I have a patch for the proposed design, will attach if there's consensus.
msg112735 - (view) Author: Fred L. Drake, Jr. (fdrake) (Python committer) Date: 2010-08-04 01:26
+1 to making support for `vars` consistent across the parser classes.

This needs to include documentation (stand-alone + docstrings) that actually make sense; the current docs require reading the code to understand what is going on.

(Generally, if you have a patch, even if there's not concensus on the feature, attaching it is a good idea.)
msg113435 - (view) Author: Fred L. Drake, Jr. (fdrake) (Python committer) Date: 2010-08-09 18:10
The getters should all accept a `default` argument that's used when no value is found in the configuration.  If the default is given, that should be returned (without conversion), instead of raising an exception.

This can be included in the same patch.
msg113472 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2010-08-09 21:07
Patch included.

New type-related unit tests have shown undesirable behaviour of read_dict() when the given option values are not strings. Support for numbers, booleans and Nones is likely to be expected so this change was introduced. `default' can hold a non-string value. `vars' were modified to accept other types as well, for consistency.
msg113483 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2010-08-09 21:57
Patch updated to never type-convert default= arguments. This behaviour is more predictable.
msg113524 - (view) Author: Fred L. Drake, Jr. (fdrake) (Python committer) Date: 2010-08-10 11:15
- Using _UNSET & similar in the docs is not good; there used to be a
  way to note a parameter as optional.  Not sure whether there is any
  more.

- Docs for methods which take vars/default should include a note
  indicating where to find the explanation, since reference docs are
  often read in a non-linear manner.

- This is a tortured way to only do something in a particular condition:

    value = str(value) if value is not None else None

  These two lines are actually more readable, since it's clear that
  the branch is a no-op:

    if value is not None:
        value = str(value)

- _unify_boolean is a strange name; doesn't parallel _unify_values at
  all.

- Having _COMPATIBLE and _UNSET defined in the classes looks more
  painful than not; no need to change for this patch.  (Should be
  changed in some later cleanup.)  Moving the whole _unify_boolean /
  _boolean_states mess to a separate top-level function that can be
  used for the conversion is attractive.
msg113529 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2010-08-10 12:28
I am away from the computer at the moment but doing my best to reply.

> - Using _UNSET & similar in the docs is not good; there used to be a
>  way to note a parameter as optional.  Not sure whether there is any
>  more.

I don't know where to look for something like it. How's 'range' documented?

> - Docs for methods which take vars/default should include a note
>  indicating where to find the explanation, since reference docs are
>  often read in a non-linear manner.

Agreed, will edit the patch.

> - This is a tortured way to only do something in a particular condition:
> 
>    value = str(value) if value is not None else None
> 
>  These two lines are actually more readable, since it's clear that
>  the branch is a no-op:
> 
>    if value is not None:
>        value = str(value)

Right. Thanks, will include this.

> - _unify_boolean is a strange name; doesn't parallel _unify_values at
>  all.

_convert_to_boolean?

> - Having _COMPATIBLE and _UNSET defined in the classes looks more
>  painful than not; no need to change for this patch.  (Should be
>  changed in some later cleanup.)  Moving the whole _unify_boolean /
>  _boolean_states mess to a separate top-level function that can be
>  used for the conversion is attractive.
> 

Will do that in a separate patch. Are you aware that moving _boolean_states out of the class is slightly backwards incompatible? I say "slightly" because it's private after all.
msg113535 - (view) Author: Fred L. Drake, Jr. (fdrake) (Python committer) Date: 2010-08-10 13:24
Even I think the leading underscore screams "don't use this from
outside, doofus!"
msg115406 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2010-09-02 22:20
Patch updated:
- _UNSET removed from docs in favor of a more natural syntax (square brackets around optional arguments)
- _COMPATIBLE left in the docs because I find less magical in that case (+ it's already covered in the docs what this special case means)
- fixed docs improperly specifying `comment_prefixes` for ConfigParser and SafeConfigParser
- docs include a remark on where to find explanation for 'vars' and 'default' in every coercing getter
- tortured code refactored
- _unify_boolean renamed to _convert_to_boolean
- _UNSET and _COMPATIBLE moved to module level (test updates)
- _boolean_states renamed to BOOLEAN_STATES (for consistency with regular expression constants)
- _convert_to_boolean and BOOLEAN_STATES left in RawConfigParser class to enable users customize them (e.g. by specifying locale-specific 'yes'/'no' pairs or things like 'enable'/'disable', etc. etc.). Customization should be instance-specific.
msg115525 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2010-09-03 22:43
On IRC Fred asked:
> Why are delimiters and comment_prefixes in the constructor signatures?

Like most configurable options, they were added to the initializer one by one (after `allow_no_value`). Only later did I notice that actually things like optionxform, _boolean_states, etc. are configurable by assignment. So this is a design decision whether one way should be preferred over the other. In a way that's unfortunate that we now have two obvious ways to do it.

I believe that keyword arguments are far better because they are more declarative, e.g.:

parser = SafeConfigParser(delimiters=(':=',),
                          comment_prefixes=('//',))

vs.

parser = SafeConfigParser()
parser._delimiters = (':=',)
parser._comment_prefixes = ('//',)

It's the way ORMs and many other frameworks do it and it feels natural.
msg115528 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-09-03 22:48
I agree with msg115525.
msg115555 - (view) Author: Fred L. Drake, Jr. (fdrake) (Python committer) Date: 2010-09-04 03:36
My question in IRC wasn't intended to mean "why are we passing settings to the constructor" so my much as "why are these configuration settings"; sorry I wasn't clear.

Have we encountered actual use cases that are not covered by the existing code?  I don't recall ever seeing a request for more flexibility in that regard.
msg115556 - (view) Author: Fred L. Drake, Jr. (fdrake) (Python committer) Date: 2010-09-04 03:43
Regardless, my concerns about including delimiters and comment_prefixes as settings is irrelevant to this issue.  The changes to them probably shouldn't have been part of this issue to begin with, but I'll try not to lose sleep over it at this point.
msg115558 - (view) Author: Fred L. Drake, Jr. (fdrake) (Python committer) Date: 2010-09-04 04:37
The patch has been commited largely as-is (r84486).

I'm not happy with the documentation yet:

- Markup like this:

    .. method:: ConfigParser.getint(section, option, raw=False,
                [vars, default])

  doesn't sit well with me.  I'm going to dig around current practice
  a bit and discuss with Georg sometime over the next few days, and
  see how best to adjust that.

- Inclusion of references to internal names in the API docs, like this
  in the signatures:

    comment_prefixes=_COMPATIBLE

  is just bad form.  Again, we need a better way.  I suspect there is
  little if any considered precedent, so we'll come up with something.

Neither of these are blockers; we can adjust this independently of the
code and doocumentation content.  (It's not part of the feature.)

Leaving this issue open for the completion of these changes.
msg115561 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2010-09-04 08:05
Actually the square bracket form is how `range()` is documented so there is some limited precedent in that regard.
History
Date User Action Args
2010-11-12 00:59:13lukasz.langasetstatus: open -> closed
2010-09-04 08:05:22lukasz.langasetmessages: + msg115561
2010-09-04 04:37:30fdrakesetresolution: accepted
messages: + msg115558
2010-09-04 03:43:34fdrakesetmessages: + msg115556
2010-09-04 03:36:57fdrakesetmessages: + msg115555
2010-09-03 22:48:30georg.brandlsetmessages: + msg115528
2010-09-03 22:43:48lukasz.langasetmessages: + msg115525
2010-09-02 22:20:25lukasz.langasetfiles: + issue9421.diff

messages: + msg115406
2010-09-02 21:47:07lukasz.langasetfiles: - issue9421.diff
2010-08-10 13:24:26fdrakesetmessages: + msg113535
2010-08-10 12:28:30lukasz.langasetmessages: + msg113529
2010-08-10 11:15:42fdrakesetmessages: + msg113524
2010-08-09 21:57:10lukasz.langasetfiles: + issue9421.diff

messages: + msg113483
2010-08-09 21:56:17lukasz.langasetfiles: - issue9421.diff
2010-08-09 21:10:00lukasz.langalinkissue6751 superseder
2010-08-09 21:08:03lukasz.langasetkeywords: + patch, needs review
files: + issue9421.diff
messages: + msg113472
2010-08-09 18:10:13fdrakesetmessages: + msg113435
2010-08-08 21:14:08lukasz.langasetassignee: lukasz.langa
2010-08-04 01:26:50fdrakesetkeywords: + easy

messages: + msg112735
2010-08-03 21:52:24lukasz.langasetnosy: + fdrake
2010-07-29 18:32:42lukasz.langacreate