Issue 1848051: issue1682942
|
Created:
13 years, 9 months ago by brian.curtin Modified:
13 years, 9 months ago Reviewers:
report Base URL:
http://svn.python.org/view/*checkout*/python/branches/py3k/ Visibility:
Public. |
Patch Set 1 #
Total comments: 19
MessagesTotal messages: 5
The tests currently do not pass because the diff file doesn't include the cfgparser.2 file. You'll have to use the "svn add" command to get the file in there. http://codereview.appspot.com/1848051/diff/1/2 File Doc/library/configparser.rst (right): http://codereview.appspot.com/1848051/diff/1/2#newcode18 Doc/library/configparser.rst:18: This module defines the classes :class:`RawConfigParser` and What about :class:`ConfigParser`? http://codereview.appspot.com/1848051/diff/1/2#newcode30 Doc/library/configparser.rst:30: at the top, followed by key/value entries, indicated by ``name`` and ``value`` I'd remove "at the top" -- being a header implies that it's at the top http://codereview.appspot.com/1848051/diff/1/2#newcode30 Doc/library/configparser.rst:30: at the top, followed by key/value entries, indicated by ``name`` and ``value`` "entries, indicated" -> "entries indicated". I don't think the comma fits there (but I'm not an English professor). http://codereview.appspot.com/1848051/diff/1/2#newcode43 Doc/library/configparser.rst:43: refer to other values in the same section, or values in a special ``DEFAULT`` I'd mention that this is called interpolation. http://codereview.appspot.com/1848051/diff/1/2#newcode77 Doc/library/configparser.rst:77: Good examples! http://codereview.appspot.com/1848051/diff/1/2#newcode79 Doc/library/configparser.rst:79: In the example above :class:`SafeConfigParser` would resolve ``%(home_dir)s`` to "above :class:" -> "above, :class" http://codereview.appspot.com/1848051/diff/1/2#newcode82 Doc/library/configparser.rst:82: ``%(home_dir)s/Pictures`` as the value of ``my_pictures``. Other features Possibly mention that RCP doesn't do interpolation here. Not a big deal though. http://codereview.appspot.com/1848051/diff/1/2#newcode86 Doc/library/configparser.rst:86: :class:`SafeConfigParser` constructor as a dictionary. Additional defaults may Initializer would be a more appropriate term than constructor http://codereview.appspot.com/1848051/diff/1/2#newcode92 Doc/library/configparser.rst:92: as will be the keys within each section. This ends up being redundant. The default dict_type is now OrderedDict so you may be better off writing this as something like... By default, sections are stored in an :class:`collections.OrderedDict` which maintains the order of all keys in a file. Alternative dictionary implementations can be supplied to the initializer. http://codereview.appspot.com/1848051/diff/1/3 File Lib/configparser.py (right): http://codereview.appspot.com/1848051/diff/1/3#newcode30 Lib/configparser.py:30: create the parser. When `defaults' is given, it is initialized into the Capitalize 'create' http://codereview.appspot.com/1848051/diff/1/3#newcode452 Lib/configparser.py:452: fp.write("%s%s%s\n" % (key, d, While changing this, might as well move to new-style "{}".format(blah) rather than %-style. Not a major issue, though. http://codereview.appspot.com/1848051/diff/1/3#newcode514 Lib/configparser.py:514: SECTCRE = re.compile(_SECT_TMPL, re.VERBOSE) I think this is a good move, using re.VERBOSE plus the changes above. http://codereview.appspot.com/1848051/diff/1/3#newcode571 Lib/configparser.py:571: if cursect is not None and optname and \ I would prefer grouping the statement with parentheses rather than splitting with `\`
Sign in to reply to this message.
http://codereview.appspot.com/1848051/diff/1/3 File Lib/configparser.py (right): http://codereview.appspot.com/1848051/diff/1/3#newcode35 Lib/configparser.py:35: When `dict_type' is given, it will be used to create the dictionary Backquotes are not usually used in docstring markup and certainly not paired with '. More common is to use *arg*, 'arg' or no markup at all.
Sign in to reply to this message.
On 2010/07/27 02:53:58, sasha wrote: > Backquotes are not usually used in docstring markup and certainly not paired > with '. More common is to use *arg*, 'arg' or no markup at all. I agree. Still, arguments to the contrary exist in the stdlib [1]. I was using notation that was there already in configparser.py. Let's leave that for now. There are more patches coming to configparser.py, and reformatting all docstrings is one go would probably be a better approach. Let's not try to fix all problems at the same time :) [1] smtpd and smtplib would be two examples on top of my head to avoid grepping :)
Sign in to reply to this message.
On 2010/07/27 01:43:16, brian.curtin wrote: > The tests currently do not pass because the diff file doesn't include the > cfgparser.2 file. You'll have to use the "svn add" command to get the file in > there. I don't have commit privileges yet. Still, the file in question was attached to the original issue: http://bugs.python.org/issue1682942 > > http://codereview.appspot.com/1848051/diff/1/2 > File Doc/library/configparser.rst (right): > > http://codereview.appspot.com/1848051/diff/1/2#newcode18 > Doc/library/configparser.rst:18: This module defines the classes > :class:`RawConfigParser` and > What about :class:`ConfigParser`? As other places in the documentation mention, ConfigParser usage is now generally discouraged because it's in fact an implementation people don't expect (it doesn't do any input validation on interpolated values, doesn't support escaping). There are probably other subtle problems but I'll get to documenting these when I finish an actual implementation. I don't think it's bad to not include ConfigParser there because the old version of docs included *only* this class which was a terrible default. > > http://codereview.appspot.com/1848051/diff/1/2#newcode30 > Doc/library/configparser.rst:30: at the top, followed by key/value entries, > indicated by ``name`` and ``value`` > I'd remove "at the top" -- being a header implies that it's at the top Done in the docs and docstrings. > > http://codereview.appspot.com/1848051/diff/1/2#newcode30 > Doc/library/configparser.rst:30: at the top, followed by key/value entries, > indicated by ``name`` and ``value`` > "entries, indicated" -> "entries indicated". I don't think the comma fits there > (but I'm not an English professor). Done. > > http://codereview.appspot.com/1848051/diff/1/2#newcode43 > Doc/library/configparser.rst:43: refer to other values in the same section, or > values in a special ``DEFAULT`` > I'd mention that this is called interpolation. > Done. > http://codereview.appspot.com/1848051/diff/1/2#newcode77 > Doc/library/configparser.rst:77: > Good examples! > :) > http://codereview.appspot.com/1848051/diff/1/2#newcode79 > Doc/library/configparser.rst:79: In the example above :class:`SafeConfigParser` > would resolve ``%(home_dir)s`` to > "above :class:" -> "above, :class" > Done. > http://codereview.appspot.com/1848051/diff/1/2#newcode82 > Doc/library/configparser.rst:82: ``%(home_dir)s/Pictures`` as the value of > ``my_pictures``. Other features > Possibly mention that RCP doesn't do interpolation here. Not a big deal though. > I do. Reformatted to make it jump out more. > http://codereview.appspot.com/1848051/diff/1/2#newcode86 > Doc/library/configparser.rst:86: :class:`SafeConfigParser` constructor as a > dictionary. Additional defaults may > Initializer would be a more appropriate term than constructor > Yes it would, the old docs mentioned constructor. Initializer sounds arcane. Maybe: "constructor" -> ":meth:`__init__` method"? Even newbies will get that one. > http://codereview.appspot.com/1848051/diff/1/2#newcode92 > Doc/library/configparser.rst:92: as will be the keys within each section. > This ends up being redundant. The default dict_type is now OrderedDict so you > may be better off writing this as something like... > > By default, sections are stored in an :class:`collections.OrderedDict` which > maintains the order of all keys in a file. Alternative dictionary > implementations can be supplied to the initializer. I've used your wording. I think the original author meant something like http://pypi.python.org/pypi/sorteddict though. He implements a simplistic SortedDict in unit tests as well. So I left the remarks about a sorted dictionary there. > > http://codereview.appspot.com/1848051/diff/1/3 > File Lib/configparser.py (right): > > http://codereview.appspot.com/1848051/diff/1/3#newcode30 > Lib/configparser.py:30: create the parser. When `defaults' is given, it is > initialized into the > Capitalize 'create' It was like that in the original docstring, all other method explanations use a small letter as well. > > http://codereview.appspot.com/1848051/diff/1/3#newcode452 > Lib/configparser.py:452: fp.write("%s%s%s\n" % (key, d, > While changing this, might as well move to new-style "{}".format(blah) rather > than %-style. Not a major issue, though. I've refactored it a bit so it now not only uses new formatting but got rid of the code duplication as well. > > http://codereview.appspot.com/1848051/diff/1/3#newcode514 > Lib/configparser.py:514: SECTCRE = re.compile(_SECT_TMPL, re.VERBOSE) > I think this is a good move, using re.VERBOSE plus the changes above. :) > > http://codereview.appspot.com/1848051/diff/1/3#newcode571 > Lib/configparser.py:571: if cursect is not None and optname and \ > I would prefer grouping the statement with parentheses rather than splitting > with `\` I specifically checked PEP8 for that case and I'm on the fence myself. Done. All updates included in the latest patch that is hitting the tracker as I'm writing this. Hope today we get to commit the thing :)
Sign in to reply to this message.
http://codereview.appspot.com/1848051/diff/1/2 File Doc/library/configparser.rst (right): http://codereview.appspot.com/1848051/diff/1/2#newcode18 Doc/library/configparser.rst:18: This module defines the classes :class:`RawConfigParser` and On 2010/07/27 01:43:16, brian.curtin wrote: > What about :class:`ConfigParser`? See #6517: we want to doc-deprecate ConfigParser. http://codereview.appspot.com/1848051/diff/1/2#newcode86 Doc/library/configparser.rst:86: :class:`SafeConfigParser` constructor as a dictionary. Additional defaults may On 2010/07/27 01:43:16, brian.curtin wrote: > Initializer would be a more appropriate term than constructor Or: “can be specified by passing them to :class:`SafeConfigParser` as a dictionary” http://codereview.appspot.com/1848051/diff/1/3 File Lib/configparser.py (right): http://codereview.appspot.com/1848051/diff/1/3#newcode118 Lib/configparser.py:118: import sre_parse Looks like you forgot to remove that. http://codereview.appspot.com/1848051/diff/1/3#newcode265 Lib/configparser.py:265: delimiters=('=', ':'), comment_prefixes=('#', ';'), I’d prefer comment_chars or comment_starts, since these are not prefixes. The mistake is made in other stdlib code, so YMMV. http://codereview.appspot.com/1848051/diff/1/3#newcode443 Lib/configparser.py:443: If `space_around_delimiters' is True (the default), delimiters Please put a blankline between the first line of the docstring (short desc) and the rest. (PEP 257 for more info)
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
