| msg52262 - (view) |
Author: Stephen Hansen (aptshansen) |
Date: 2007-03-18 02:54 |
| This patch is in response to 1165404, which I was reviewing. It turns out I actually rather like the idea, since it would have made my life easier in a current project to not have to totally roll my own ConfigParser to read an external configuration file. So! I updated the previous patch to apply against the current HEAD, and then added some documentation and test cases (and made sure it didn't break existing test cases) I did alter the previously proposed API a touch too; 'delimiters' instead of 'delimiter' as its taking a list, and to actually take a sequence instead of a string containing characters because IMHO it just looks cleaner. Passing in ":=" as indicating either : or = are delimiters looks odd ot me (Hi, Pascal), it looks like it'd make the _string_ of ":=" the delimiter. I also adjusted the names slightly to fit into the style of the module. ('_delimiter' instead of 'delim'). I also updated the docstring on the module to document dict_type being an option (it wasn't before) My text editor seems to think the documentation is OK LaTeX-- but until uh, two days ago, I never touched LaTeX, so someone should eye that :) |
|
|
| msg104847 - (view) |
Author: Tres Seaver (tseaver) * |
Date: 2010-05-03 15:27 |
| I'm afraid the patch no longer applies cleanly to the trunk, although at least updating the docs should be easier now that they are converted to ReStructuredText. The tests in the patch for the new feature seem sensible. |
|
|
| msg110575 - (view) |
Author: Mark Lawrence (BreamoreBoy) * |
Date: 2010-07-17 16:19 |
| Could the patch be reworked for 3.2? |
|
|
| msg111407 - (view) |
Author: Łukasz Langa (lukasz.langa) *  |
Date: 2010-07-24 01:31 |
| Part of the patch has already been independently implemented (support for custom `dict_type`s). As for the custom delimiters the patch is somewhat incomplete. Let's take for instance delimiting by space characters (e.g. `delimiters = (" ", )`). ConfigParser would input so it would be quite bizarre. Brett, I have a patch ready but it depends on changes I've made. |
|
|
| msg111501 - (view) |
Author: Mark Lawrence (BreamoreBoy) * |
Date: 2010-07-24 18:40 |
| Łukasz please attach your patch and I'll run with it, while you're at it could you also provide a unit test patch for #5412, thanks. |
|
|
| msg111614 - (view) |
Author: Michael Foord (michael.foord) * |
Date: 2010-07-26 13:38 |
| This would change the format of config files that configparser supports. Should there be some discussion of this on python-dev first? The patch for the docs is against the latex docs, so definitely needs updating. |
|
|
| msg111657 - (view) |
Author: Łukasz Langa (lukasz.langa) * |
Date: 2010-07-26 18:04 |
| The original patch was of no use because didn't let users specify multicharacter delimiters and comment delimiters. The main goal behind the implementation was making monstrosities like the Samba configuration file work. This is why a sample smb.conf is added to the tests as to show what is now parsable. The patch is called .diff and is quite big. Georg, Brian, before you panic, please note the following: the old test suite with the new code works (which means it's by default backwards compatible). With a patch of that size it also might just be easier for you to browse the patched code and not the patch itself. The Doc/ part still needs work but after 10 hours of coding I have enough for today :) |
|
|
| msg111671 - (view) |
Author: Łukasz Langa (lukasz.langa) * |
Date: 2010-07-26 22:44 |
| Updated the patch after review by Georg Brandl. This version includes documentation updates as well. |
|
|
| msg111678 - (view) |
Author: Brian Curtin (brian.curtin) * |
Date: 2010-07-27 01:45 |
| I uploaded the current patch to Rietveld and reviewed it there, CC'ed Łukasz. http://codereview.appspot.com/1848051/show is the link. I only gave the tests a once-over since they failed for not having the test file Łukasz meant to include. I'll review those more in depth on the next patch. |
|
|
| msg111693 - (view) |
Author: Łukasz Langa (lukasz.langa) * |
Date: 2010-07-27 10:00 |
| Updated the patch after review by Brian Curtin and Alexander Belopolsky. All remarks addressed, I think it's ready for inclusion. |
|
|
| msg111775 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2010-07-28 08:32 |
| Looks good to me. |
|
|
| msg111783 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-07-28 10:27 |
| I made some minor remarks on rietveld, it seems they’re saved but no email has come here. |
|
|
| msg111785 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-07-28 10:53 |
| Ah, the tracker does not know the address I use for Google, sorry. My comments are visible on Rietveld. |
|
|
| msg111789 - (view) |
Author: Łukasz Langa (lukasz.langa) * |
Date: 2010-07-28 11:15 |
| New patch after review by Éric Araujo. The difference between the last one and the current is cosmetic: --- Lib/configparser.py 2010-07-27 11:36:51.000000000 +0200 +++ Lib/configparser.py.2 2010-07-28 13:05:39.000000000 +0200 @@ -117,3 +117,2 @@ import re -import sre_parse import sys @@ -140,2 +139,3 @@ BaseException.""" + return self.__message @@ -145,2 +145,3 @@ BaseException.""" + self.__message = value @@ -301,2 +302,3 @@ """ + if section.lower() == "default": @@ -338,2 +340,3 @@ """ + if isinstance(filenames, str): @@ -358,4 +361,4 @@ used. - """ + if filename is None: @@ -419,2 +422,3 @@ """Check for the existence of a given option in a given section.""" + if not section or section == DEFAULTSECT: @@ -431,2 +435,3 @@ """Set an option.""" + if not section or section == DEFAULTSECT: @@ -444,2 +449,3 @@ between keys and values are surrounded by spaces.""" + if space_around_delimiters: @@ -456,2 +462,3 @@ """Remove an option.""" + if not section or section == DEFAULTSECT: @@ -471,2 +478,3 @@ """Remove a file section.""" + existed = section in self._sections @@ -529,2 +537,3 @@ """ + cursect = None # None, or a dictionary @@ -657,2 +666,3 @@ """ + d = self._defaults.copy() @@ -690,2 +700,3 @@ """ + d = self._defaults.copy() @@ -789,2 +800,3 @@ """Set an option. Extend ConfigParser.set: check for string values.""" + # The only legal non-string value if we allow valueless Some remarks on Éric's review: - thanks for reviewing the patch, however you did review an outdated version - comment_prefixes are by all means prefixes. Collins dictionary: "prefix, n. - 2. something coming or placed before". Other reviewers didn't pose the current name as inapt - other remarks corrected in the current patch |
|
|
| msg111791 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-07-28 11:32 |
| I advised putting a blankline between the first line of a docstring and the rest of it, not between the docstring and the body of the function. Re. prefix, Wordnet is more precise than Collins here: “an affix that is added in front of the word”. A prefix is not just a sequence of characters, it’s a morpheme (building block for word, if you want). So prefix is a wrong name in the docstring of str.startswith, since e.g. “pyt” is not a prefix, whereas “out” is, but str.startswith works with any sequence, not only something that is a prefix. > Other reviewers didn't pose the current name as inapt They’re not linguists. Not saying this is a good or bad thing. :) Using linguistics terms to speak of computer languages is often a bad thing, but in this case I’d like to avoid using prefix. |
|
|
| msg111803 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2010-07-28 13:14 |
| Committed an extensively edited patch in r83202. Thanks all! |
|
|