Message145698
Thanks for the patch. It mostly looks good; a detailed review follow.
+ The constructor takes two arguments, the first is the template string and
+ the second (optional) is a dictionary object which specify which values
+ should be used as default values, if no provided.
Just say “a dictionary”, or even “a mapping”. There’s also a grammar typo and a bit of redundancy, so I propose: “is a mapping which gives default values”. What do you think about adding a small example in the examples section later in the file? It would probably be clearer than English.
Like :meth:`substitute`, except that if placeholders are missing from
- *mapping* and *kwds*, instead of raising a :exc:`KeyError` exception, the
- original placeholder will appear in the resulting string intact. Also,
- unlike with :meth:`substitute`, any other appearances of the ``$`` will
- simply return ``$`` instead of raising :exc:`ValueError`.
+ *mapping*, *kwds* and *default*, instead of raising a :exc:`KeyError`
default is not an argument, so *foo* markup is misleading. Either use “the default values given to the constructor” or just “self.default”.
+ exception, the original placeholder will appear in the resulting string
+ intact. Also, unlike with :meth:`substitute`, any other appearances of
+ the ``$`` will simply return ``$`` instead of raising :exc:`ValueError`.
If you don’t mind, I prefer if you have a few very short or too long lines if that helps reducing diff noise (i.e. lines that appear in the diff but are not changed, only rewrapped).
+ .. attribute:: default
+
+ This is the optional dictionary object passed to the constructor's
+ *template* argument.
I’m not a native English speaker, but “passed to” seems wrong here (and in the other attribute’s doc). I’d say “passed as the *default* argument”.
- def __init__(self, template):
+ def __init__(self, template, default={}):
Binding a mutable object to a local name at compile time is not good: All instances created without *default* argument will share the same dict, so editions to onetemplate.default will change anothertemplate.default too. The common idiom is to use None as default value and add this:
self.default = default if default is not None else {} |
|
Date |
User |
Action |
Args |
2011-10-17 14:11:33 | eric.araujo | set | recipients:
+ eric.araujo, nitupho |
2011-10-17 14:11:33 | eric.araujo | set | messageid: <1318860693.4.0.119480702638.issue13173@psf.upfronthosting.co.za> |
2011-10-17 14:11:32 | eric.araujo | link | issue13173 messages |
2011-10-17 14:11:31 | eric.araujo | create | |
|