This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author eric.araujo
Recipients eric.araujo, nitupho
Date 2011-10-17.14:11:31
SpamBayes Score 7.2164497e-14
Marked as misclassified No
Message-id <1318860693.4.0.119480702638.issue13173@psf.upfronthosting.co.za>
In-reply-to
Content
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 {}
History
Date User Action Args
2011-10-17 14:11:33eric.araujosetrecipients: + eric.araujo, nitupho
2011-10-17 14:11:33eric.araujosetmessageid: <1318860693.4.0.119480702638.issue13173@psf.upfronthosting.co.za>
2011-10-17 14:11:32eric.araujolinkissue13173 messages
2011-10-17 14:11:31eric.araujocreate