Éric Araujo <[email protected]> added the comment:
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 {}
----------
_______________________________________
Python tracker <[email protected]>
<http://bugs.python.org/issue13173>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
