msg111986 - (view) |
Author: Łukasz Langa (lukasz.langa) * |
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 Drake (fdrake) |
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 Drake (fdrake) |
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) * |
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) * |
Date: 2010-08-09 21:57 |
Patch updated to never type-convert default= arguments. This behaviour is more predictable.
|
msg113524 - (view) |
Author: Fred Drake (fdrake) |
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) * |
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 Drake (fdrake) |
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) * |
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) * |
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) * |
Date: 2010-09-03 22:48 |
I agree with msg115525.
|
msg115555 - (view) |
Author: Fred Drake (fdrake) |
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 Drake (fdrake) |
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 Drake (fdrake) |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:04 | admin | set | github: 53667 |
2010-11-12 00:59:13 | lukasz.langa | set | status: open -> closed |
2010-09-04 08:05:22 | lukasz.langa | set | messages:
+ msg115561 |
2010-09-04 04:37:30 | fdrake | set | resolution: accepted messages:
+ msg115558 |
2010-09-04 03:43:34 | fdrake | set | messages:
+ msg115556 |
2010-09-04 03:36:57 | fdrake | set | messages:
+ msg115555 |
2010-09-03 22:48:30 | georg.brandl | set | messages:
+ msg115528 |
2010-09-03 22:43:48 | lukasz.langa | set | messages:
+ msg115525 |
2010-09-02 22:20:25 | lukasz.langa | set | files:
+ issue9421.diff
messages:
+ msg115406 |
2010-09-02 21:47:07 | lukasz.langa | set | files:
- issue9421.diff |
2010-08-10 13:24:26 | fdrake | set | messages:
+ msg113535 |
2010-08-10 12:28:30 | lukasz.langa | set | messages:
+ msg113529 |
2010-08-10 11:15:42 | fdrake | set | messages:
+ msg113524 |
2010-08-09 21:57:10 | lukasz.langa | set | files:
+ issue9421.diff
messages:
+ msg113483 |
2010-08-09 21:56:17 | lukasz.langa | set | files:
- issue9421.diff |
2010-08-09 21:10:00 | lukasz.langa | link | issue6751 superseder |
2010-08-09 21:08:03 | lukasz.langa | set | keywords:
+ patch, needs review files:
+ issue9421.diff messages:
+ msg113472
|
2010-08-09 18:10:13 | fdrake | set | messages:
+ msg113435 |
2010-08-08 21:14:08 | lukasz.langa | set | assignee: lukasz.langa |
2010-08-04 01:26:50 | fdrake | set | keywords:
+ easy
messages:
+ msg112735 |
2010-08-03 21:52:24 | lukasz.langa | set | nosy:
+ fdrake
|
2010-07-29 18:32:42 | lukasz.langa | create | |