#1521950: shlex.split() does not tokenize like the shell
|
Created:
1 year, 5 months ago by robodan Modified:
1 year ago Reviewers:
merwok, vinay_sajip, rdmurray CC:
Vinay Sajip, eric.smith, robodan_users.sourceforge.net, ezio.melotti, eric.araujo, r.david.murray Visibility:
Public. |
Patch Set 1 #
Total comments: 7
Patch Set 2 #
Total comments: 30
Patch Set 3 #
Total comments: 21
Patch Set 4 #
Created: 1 year ago
MessagesTotal messages: 11
Great patch. http://bugs.python.org/review/1521950/diff/3939/13004 File Lib/shlex.py (right): http://bugs.python.org/review/1521950/diff/3939/13004#newcode52 Lib/shlex.py:52: if not control: I wonder if this is the best name for the argument, given that it may also be an iterable. I’m not great for names; I think “terminal_tokens” is more descriptive of what it’s used for and what it accepts, but it’s not beautiful. What do you think? http://bugs.python.org/review/1521950/diff/3939/13004#newcode58 Lib/shlex.py:58: self.pbchars = deque() Could you add a comment or find a more descriptive name for this? Also, if it’s a new public attribute it deserves documentation. http://bugs.python.org/review/1521950/diff/3939/13005 File Lib/test/test_shlex.py (right): http://bugs.python.org/review/1521950/diff/3939/13005#newcode186 Lib/test/test_shlex.py:186: ref = ['echo', 'hi', delimiter, 'echo', 'bye'] Ah, I didn’t imagine that the control tokens would not be present in the result! Isn’t this a problem?
Sign in to reply to this message.
BTW, I never got an email from the review tool from your comments! Just happened upon them when I was looking at this issue again. http://bugs.python.org/review/1521950/diff/3939/13005 File Lib/test/test_shlex.py (right): http://bugs.python.org/review/1521950/diff/3939/13005#newcode186 Lib/test/test_shlex.py:186: ref = ['echo', 'hi', delimiter, 'echo', 'bye'] On 2012/01/07 15:54:26, eric.araujo wrote: > Ah, I didn’t imagine that the control tokens would not be present in the result! They *are* present - the third element of the ref list is the control chars token. Or did I misunderstand your comment?
Sign in to reply to this message.
http://bugs.python.org/review/1521950/diff/3939/13004 File Lib/shlex.py (right): http://bugs.python.org/review/1521950/diff/3939/13004#newcode52 Lib/shlex.py:52: if not control: On 2012/01/07 15:54:26, eric.araujo wrote: > I wonder if this is the best name for the argument, given that it may also be an > iterable. I’m not great for names; I think “terminal_tokens” is more > descriptive of what it’s used for and what it accepts, but it’s not beautiful. > What do you think? I agree it's not beautiful. I would prefer "terminal_chars" or "punctuation_chars" to "terminal_tokens" as it's really indicating which chars are punctuation. http://bugs.python.org/review/1521950/diff/3939/13004#newcode58 Lib/shlex.py:58: self.pbchars = deque() On 2012/01/07 15:54:26, eric.araujo wrote: > Could you add a comment or find a more descriptive name for this? Also, if it’s > a new public attribute it deserves documentation. I'm not sure it needs to be public - could rename to "_pushback_chars". Comment would be "Push back queue used by lexical scanner lookahead logic".
Sign in to reply to this message.
http://bugs.python.org/review/1521950/diff/3939/13005 File Lib/test/test_shlex.py (right): http://bugs.python.org/review/1521950/diff/3939/13005#newcode186 Lib/test/test_shlex.py:186: ref = ['echo', 'hi', delimiter, 'echo', 'bye'] > They *are* present - the third element of the ref list is the > control chars token. Or did I misunderstand your comment? Ah, it looks like I overlooked that! Everything’s fine.
Sign in to reply to this message.
I've only really reviewed the docs (and thus the API) and have some concerns. I'd have to spend more time than I have right now to do a good code review. http://bugs.python.org/review/1521950/diff/4234/14802 File Doc/library/shlex.rst (right): http://bugs.python.org/review/1521950/diff/4234/14802#newcode73 Doc/library/shlex.rst:73: .. class:: shlex(instream=None, infile=None, posix=False, punctuation_chars=False) When I first read this I did not understand punctuation_chars dual type value. This needs to be made clear right up front (see below). http://bugs.python.org/review/1521950/diff/4234/14802#newcode85 Doc/library/shlex.rst:85: will try to be as close as possible to the POSIX shell parsing rules. punctuation_chars should also be discussed in the text like the other arguments are, at which point the second sentence of the versionchanged can be deleted. http://bugs.python.org/review/1521950/diff/4234/14802#newcode220 Doc/library/shlex.rst:220: .. attribute:: shlex.whitespace_split The docs for this attribute probably need adjustment, or at least an example, since it is not obvious how this interacts with punctuation_chars from the current text. http://bugs.python.org/review/1521950/diff/4234/14802#newcode282 Doc/library/shlex.rst:282: .. attribute:: shlex.token_type This should come after the description of shlex.token and indicate that it is the type of the token currently in the token buffer (assuming it is, the code confuses me and I haven't finished figuring it out yet). Oh, looking at the tests, I'm guessing that the token_type is the type of the token last returned. I'm not sure I like that, given the ability to push back tokens. And obviously "will reflect of type of the token returned" didn't say that to me for some reason. http://bugs.python.org/review/1521950/diff/4234/14802#newcode347 Doc/library/shlex.rst:347: The :class:`shlex` class now provides added compatibility with the parsing This language is not future-proof. It should simply describe the features as existing not as "added". Only the versionchanged text string should discuss the addition as an addition. http://bugs.python.org/review/1521950/diff/4234/14802#newcode361 Doc/library/shlex.rst:361: for punct, message in ((False, 'Old'), (True, 'New')): I find this example to be very confusing. I had to read it a couple times to understand that "old" and "new" were print decoration and not something to do with controlling what happened. http://bugs.python.org/review/1521950/diff/4234/14802#newcode374 Doc/library/shlex.rst:374: Instead of passing ``True`` as the value for the punctuation_chars parameter, Oh, so you can pass a string. This needs to be mentioned earlier to motivate the name choice, I think. I don't like True/False/str as an argument type combination, but I suppose backward compatibility is an issue here. http://bugs.python.org/review/1521950/diff/4234/14802#newcode383 Doc/library/shlex.rst:383: .. note:: When ``punctuation_chars`` is specified, the :attr:`~shlex.wordchars` "``punctuation_chars`` is specified" should be "``punctionation_chars`` is not False, I think. http://bugs.python.org/review/1521950/diff/4234/14802#newcode384 Doc/library/shlex.rst:384: attribute is augmented with the characters ``~-./*?=``. That is because these OK, this is getting even more confusing. Reading this doc I would think, "why aren't all characters that aren't punctuation word chars?" (which *might* solve the shlex unicode issue). At the very least the behavior with respect to non-punctuation non-word characters should be explained (they go in their own token? Why? :)
Sign in to reply to this message.
My response to R. David Murray's comments. http://bugs.python.org/review/1521950/diff/4234/14802 File Doc/library/shlex.rst (right): http://bugs.python.org/review/1521950/diff/4234/14802#newcode73 Doc/library/shlex.rst:73: .. class:: shlex(instream=None, infile=None, posix=False, punctuation_chars=False) On 2012/04/23 03:02:12, r.david.murray wrote: > When I first read this I did not understand punctuation_chars dual type value. > This needs to be made clear right up front (see below). Ok, I will add the description of punctuation_chars to the text here. http://bugs.python.org/review/1521950/diff/4234/14802#newcode85 Doc/library/shlex.rst:85: will try to be as close as possible to the POSIX shell parsing rules. On 2012/04/23 03:02:12, r.david.murray wrote: > punctuation_chars should also be discussed in the text like the other arguments > are, at which point the second sentence of the versionchanged can be deleted. I will move the "see ..." reference to the text I add about punctuation_chars above. http://bugs.python.org/review/1521950/diff/4234/14802#newcode220 Doc/library/shlex.rst:220: .. attribute:: shlex.whitespace_split On 2012/04/23 03:02:12, r.david.murray wrote: > The docs for this attribute probably need adjustment, or at least an example, > since it is not obvious how this interacts with punctuation_chars from the > current text. I propose to add: "If this attribute is True, punctuation_chars will have no effect, and splitting will happen only in whitespaces. When using punctuation_chars, which is intended to provide parsing closer to that implemented by shells, it is advisable to leave whitespace_split as False (the default value)." http://bugs.python.org/review/1521950/diff/4234/14802#newcode282 Doc/library/shlex.rst:282: .. attribute:: shlex.token_type On 2012/04/23 03:02:12, r.david.murray wrote: > This should come after the description of shlex.token and indicate that it is > the type of the token currently in the token buffer (assuming it is, the code > confuses me and I haven't finished figuring it out yet). > > Oh, looking at the tests, I'm guessing that the token_type is the type of the > token last returned. I'm not sure I like that, given the ability to push back > tokens. And obviously "will reflect of type of the token returned" didn't say > that to me for some reason. I will change "This will reflect the type of token returned" to "The type of the token last read by read_token. Note that get_token may either call read_token, or just return a previously pushed back token; hence, following a call to get_token, the token_type attribute may not match the token returned by get_token." http://bugs.python.org/review/1521950/diff/4234/14802#newcode347 Doc/library/shlex.rst:347: The :class:`shlex` class now provides added compatibility with the parsing On 2012/04/23 03:02:12, r.david.murray wrote: > This language is not future-proof. It should simply describe the features as > existing not as "added". Only the versionchanged text string should discuss the > addition as an addition. I will address this. http://bugs.python.org/review/1521950/diff/4234/14802#newcode361 Doc/library/shlex.rst:361: for punct, message in ((False, 'Old'), (True, 'New')): On 2012/04/23 03:02:12, r.david.murray wrote: > I find this example to be very confusing. I had to read it a couple times to > understand that "old" and "new" were print decoration and not something to do > with controlling what happened. I'll change it to "for punct in (False, True):" and derive message in the loop from punct's value. http://bugs.python.org/review/1521950/diff/4234/14802#newcode374 Doc/library/shlex.rst:374: Instead of passing ``True`` as the value for the punctuation_chars parameter, On 2012/04/23 03:02:12, r.david.murray wrote: > Oh, so you can pass a string. This needs to be mentioned earlier to motivate > the name choice, I think. I don't like True/False/str as an argument type > combination, but I suppose backward compatibility is an issue here. Yes: False -> old behaviour, True -> new behaviour with default punctuation chars, string -> new behaviour with user defined punctuation chars. This will be added to the description of punctuation_chars which I add to the constructor documentation. http://bugs.python.org/review/1521950/diff/4234/14802#newcode383 Doc/library/shlex.rst:383: .. note:: When ``punctuation_chars`` is specified, the :attr:`~shlex.wordchars` On 2012/04/23 03:02:12, r.david.murray wrote: > "``punctuation_chars`` is specified" should be "``punctionation_chars`` is not > False, I think. It should be "is True" rather than "not False", because if you specify a string, that will be taken to be the list of punctuation chars. http://bugs.python.org/review/1521950/diff/4234/14802#newcode384 Doc/library/shlex.rst:384: attribute is augmented with the characters ``~-./*?=``. That is because these On 2012/04/23 03:02:12, r.david.murray wrote: > OK, this is getting even more confusing. Reading this doc I would think, "why > aren't all characters that aren't punctuation word chars?" (which *might* solve > the shlex unicode issue). At the very least the behavior with respect to > non-punctuation non-word characters should be explained (they go in their own > token? Why? :) This would require a bigger refactoring of the read_token procedure than I felt comfortable with (in terms of being able to guarantee full backward compatibility, yet not spending too long on the refactored implementation) :-)
Sign in to reply to this message.
After you address the doc issues with a new patch and some decision is made about token_type, I'll do a code review. http://bugs.python.org/review/1521950/diff/4234/14802 File Doc/library/shlex.rst (right): http://bugs.python.org/review/1521950/diff/4234/14802#newcode220 Doc/library/shlex.rst:220: .. attribute:: shlex.whitespace_split That sounds good. http://bugs.python.org/review/1521950/diff/4234/14802#newcode282 Doc/library/shlex.rst:282: .. attribute:: shlex.token_type That strikes me as a potentially error prone interface. What about instead providing a function token_type that takes any token and returns the appropriate (computed) type? If I understand correctly the type can be computed based just on the punctuation_chars and wordchars lists. That would also eliminate the setting of token_type in the code, which is the part that I found the most confusing to follow. http://bugs.python.org/review/1521950/diff/4234/14802#newcode383 Doc/library/shlex.rst:383: .. note:: When ``punctuation_chars`` is specified, the :attr:`~shlex.wordchars` On 2012/04/23 11:42:09, Vinay Sajip wrote: > On 2012/04/23 03:02:12, r.david.murray wrote: > > "``punctuation_chars`` is specified" should be "``punctionation_chars`` is not > > False, I think. > > It should be "is True" rather than "not False", because if you specify a string, > that will be taken to be the list of punctuation chars. If your code only added the extra chars to wordchars if punctuation_chars was True, that would be surprising to the user, I think. It's not what your code does, though. However, that brings up something I hadn't though of previously. When punctuation_chars is a string you probably need to remove any characters in it from the list of chars you add to wordchars, and note that in the docs for completeness. http://bugs.python.org/review/1521950/diff/4234/14802#newcode384 Doc/library/shlex.rst:384: attribute is augmented with the characters ``~-./*?=``. That is because these On 2012/04/23 11:42:09, Vinay Sajip wrote: > On 2012/04/23 03:02:12, r.david.murray wrote: > > OK, this is getting even more confusing. Reading this doc I would think, "why > > aren't all characters that aren't punctuation word chars?" (which *might* > solve > > the shlex unicode issue). At the very least the behavior with respect to > > non-punctuation non-word characters should be explained (they go in their own > > token? Why? :) > > This would require a bigger refactoring of the read_token procedure than I felt > comfortable with (in terms of being able to guarantee full backward > compatibility, yet not spending too long on the refactored implementation) :-) OK, let us leave that refactoring to the shlex-unicode issue, then.
Sign in to reply to this message.
http://bugs.python.org/review/1521950/diff/4234/14802 File Doc/library/shlex.rst (right): http://bugs.python.org/review/1521950/diff/4234/14802#newcode73 Doc/library/shlex.rst:73: .. class:: shlex(instream=None, infile=None, posix=False, punctuation_chars=False) On 2012/04/23 11:42:09, Vinay Sajip wrote: > Ok, I will add the description of punctuation_chars to the text here. Done. http://bugs.python.org/review/1521950/diff/4234/14802#newcode85 Doc/library/shlex.rst:85: will try to be as close as possible to the POSIX shell parsing rules. On 2012/04/23 11:42:09, Vinay Sajip wrote: > I will move the "see ..." reference to the text I add about punctuation_chars > above. Done. http://bugs.python.org/review/1521950/diff/4234/14802#newcode220 Doc/library/shlex.rst:220: .. attribute:: shlex.whitespace_split On 2012/04/23 11:42:09, Vinay Sajip wrote: > I propose to add: "If this attribute is True, punctuation_chars will have no > effect, and splitting will happen only in whitespaces. When using > punctuation_chars, which is intended to provide parsing closer to that > implemented by shells, it is advisable to leave whitespace_split as False (the > default value)." Done. http://bugs.python.org/review/1521950/diff/4234/14802#newcode282 Doc/library/shlex.rst:282: .. attribute:: shlex.token_type On 2012/04/23 14:20:13, r.david.murray wrote: > What about instead providing a function token_type that takes any token and > returns the appropriate (computed) type? If I understand correctly the type can > be computed based just on the punctuation_chars and wordchars lists. That would > also eliminate the setting of token_type in the code, which is the part that I > found the most confusing to follow. I will just remove this completely, and not provide token_type at all. Users can test if "token[0] in shlexobj.punctuation_chars" to see if a token is punctuation or not. http://bugs.python.org/review/1521950/diff/4234/14802#newcode347 Doc/library/shlex.rst:347: The :class:`shlex` class now provides added compatibility with the parsing On 2012/04/23 11:42:09, Vinay Sajip wrote: > > I will address this. > Done. http://bugs.python.org/review/1521950/diff/4234/14802#newcode361 Doc/library/shlex.rst:361: for punct, message in ((False, 'Old'), (True, 'New')): On 2012/04/23 11:42:09, Vinay Sajip wrote: > I'll change it to "for punct in (False, True):" and derive message in the loop > from punct's value. Done. http://bugs.python.org/review/1521950/diff/4234/14802#newcode374 Doc/library/shlex.rst:374: Instead of passing ``True`` as the value for the punctuation_chars parameter, On 2012/04/23 11:42:09, Vinay Sajip wrote: > Yes: False -> old behaviour, True -> new behaviour with default punctuation > chars, string -> new behaviour with user defined punctuation chars. This will be > added to the description of punctuation_chars which I add to the constructor > documentation. Done. http://bugs.python.org/review/1521950/diff/4234/14802#newcode383 Doc/library/shlex.rst:383: .. note:: When ``punctuation_chars`` is specified, the :attr:`~shlex.wordchars` On 2012/04/23 14:20:13, r.david.murray wrote: > If your code only added the extra chars to wordchars if punctuation_chars was > True, that would be surprising to the user, I think. It's not what your code > does, though. Well, it only does that addition "if punctuation_chars:" > However, that brings up something I hadn't though of previously. When > punctuation_chars is a string you probably need to remove any characters in it > from the list of chars you add to wordchars, and note that in the docs for > completeness. > Added.
Sign in to reply to this message.
http://bugs.python.org/review/1521950/diff/4748/16844 File Doc/library/shlex.rst (right): http://bugs.python.org/review/1521950/diff/4748/16844#newcode87 Doc/library/shlex.rst:87: even closer behaviour to how reall shells parse, the *punctuation_chars* s/reall/real/ when I next change this.
Sign in to reply to this message.
I haven't wrapped my head around the shlex parsing algorithm, so I haven't really evaluated your algorithm changes. Since the tests for that part look reasonable, though, I'm assuming your algorithm changes are OK :) http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst File Doc/library/shlex.rst (right): http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode84 Doc/library/shlex.rst:84: mode: when *posix* is not true (default), the :class:`shlex` instance will When you edit this next could you change "not true" to "false"? The negative is confusing. http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode87 Doc/library/shlex.rst:87: even closer behaviour to how reall shells parse, the *punctuation_chars* On 2012/04/26 22:08:08, Vinay Sajip wrote: > s/reall/real/ when I next change this. I would remove the passive voice here. How about "The *punctuation_chars* argument provides a way to make the behavior even closer to how real shells parse." http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode92 Doc/library/shlex.rst:92: single token. You can also provide a non-empty string of characters, which To keep the paragraph parallel, this should be "If set to a non-empty string of characters, those characters will be used as the punctuation characters." http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode95 Doc/library/shlex.rst:95: :ref:`improved-shell-compatibility` for more information. If I understand the next change correctly, this should really read "Any characters in the :attr:`wordchars` attribute that appear in *punctuation_chars* will be removed from :attr:`wordchars`." http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode238 Doc/library/shlex.rst:238: example, for parsing command lines with :class:`shlex`, getting tokens in a Since whitespace_split=True is no longer the best way to get one closer to parsing like the shell does, I think we should just eliminate the "This is useful" sentence. http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode398 Doc/library/shlex.rst:398: .. note:: When ``punctuation_chars`` is specified, the :attr:`~shlex.wordchars` I don't think this needs to be a note, but that's a judgement call so I'm fine if you leave it as one. http://bugs.python.org/review/1521950/diff/4748/Lib/shlex.py File Lib/shlex.py (right): http://bugs.python.org/review/1521950/diff/4748/Lib/shlex.py#newcode21 Lib/shlex.py:21: def __init__(self, instream=None, infile=None, posix=False, punctuation_chars=False): Please wrap this line to < 80 chars. http://bugs.python.org/review/1521950/diff/4748/Lib/shlex.py#newcode66 Lib/shlex.py:66: self.wordchars.remove(c) This should be: self.wordchars = ''.join(c for c in self.wordchars if c not in self.punctuation_chars) The fact that you missed this indicates you are missing a test :) http://bugs.python.org/review/1521950/diff/4748/Lib/shlex.py#newcode213 Lib/shlex.py:213: and nextchar != escapedstate): I think it would be better to indent the 'and' further (I believe that's what PEP8 calls for...actually I think PEP8 would also have the previous line ending with the operator rather than this line beginning with one.) http://bugs.python.org/review/1521950/diff/4748/Lib/test/test_shlex.py File Lib/test/test_shlex.py (right): http://bugs.python.org/review/1521950/diff/4748/Lib/test/test_shlex.py#newcod... Lib/test/test_shlex.py:256: self.assertEqual(observed, expected) As mentioned in the shlex review, you are missing a test for punctuation chars where a punc char appears in wordchars. There should also be at least one test with both punctuation_chars and whitespace_split=True.
Sign in to reply to this message.
New patch set with these changes is now available :-) http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst File Doc/library/shlex.rst (right): http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode84 Doc/library/shlex.rst:84: mode: when *posix* is not true (default), the :class:`shlex` instance will On 2012/06/02 21:58:30, r.david.murray wrote: > When you edit this next could you change "not true" to "false"? The negative is > confusing. Done. http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode87 Doc/library/shlex.rst:87: even closer behaviour to how reall shells parse, the *punctuation_chars* On 2012/06/02 21:58:30, r.david.murray wrote: > On 2012/04/26 22:08:08, Vinay Sajip wrote: > > s/reall/real/ when I next change this. > > > I would remove the passive voice here. How about "The *punctuation_chars* > argument provides a way to make the behavior even closer to how real shells > parse." Done. http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode92 Doc/library/shlex.rst:92: single token. You can also provide a non-empty string of characters, which On 2012/06/02 21:58:30, r.david.murray wrote: > To keep the paragraph parallel, this should be "If set to a non-empty string of > characters, those characters will be used as the punctuation characters." Done. http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode95 Doc/library/shlex.rst:95: :ref:`improved-shell-compatibility` for more information. On 2012/06/02 21:58:30, r.david.murray wrote: > If I understand the next change correctly, this should really read "Any > characters in the :attr:`wordchars` attribute that appear in *punctuation_chars* > will be removed from :attr:`wordchars`." Done. http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode238 Doc/library/shlex.rst:238: example, for parsing command lines with :class:`shlex`, getting tokens in a On 2012/06/02 21:58:30, r.david.murray wrote: > Since whitespace_split=True is no longer the best way to get one closer to > parsing like the shell does, I think we should just eliminate the "This is > useful" sentence. Done. http://bugs.python.org/review/1521950/diff/4748/Doc/library/shlex.rst#newcode398 Doc/library/shlex.rst:398: .. note:: When ``punctuation_chars`` is specified, the :attr:`~shlex.wordchars` On 2012/06/02 21:58:30, r.david.murray wrote: > I don't think this needs to be a note, but that's a judgement call so I'm fine > if you leave it as one. I'll leave it as is - I made it a note to stand out a little, because the side-effect might be unexpected. http://bugs.python.org/review/1521950/diff/4748/Lib/shlex.py File Lib/shlex.py (right): http://bugs.python.org/review/1521950/diff/4748/Lib/shlex.py#newcode21 Lib/shlex.py:21: def __init__(self, instream=None, infile=None, posix=False, punctuation_chars=False): On 2012/06/02 21:58:30, r.david.murray wrote: > Please wrap this line to < 80 chars. Done. http://bugs.python.org/review/1521950/diff/4748/Lib/shlex.py#newcode66 Lib/shlex.py:66: self.wordchars.remove(c) On 2012/06/02 21:58:30, r.david.murray wrote: > This should be: > > self.wordchars = ''.join(c for c in self.wordchars if c not in > self.punctuation_chars) > > The fact that you missed this indicates you are missing a test :) Done. http://bugs.python.org/review/1521950/diff/4748/Lib/shlex.py#newcode213 Lib/shlex.py:213: and nextchar != escapedstate): On 2012/06/02 21:58:30, r.david.murray wrote: > I think it would be better to indent the 'and' further (I believe that's what > PEP8 calls for...actually I think PEP8 would also have the previous line ending > with the operator rather than this line beginning with one.) Done. http://bugs.python.org/review/1521950/diff/4748/Lib/test/test_shlex.py File Lib/test/test_shlex.py (right): http://bugs.python.org/review/1521950/diff/4748/Lib/test/test_shlex.py#newcod... Lib/test/test_shlex.py:256: self.assertEqual(observed, expected) On 2012/06/02 21:58:30, r.david.murray wrote: > As mentioned in the shlex review, you are missing a test for punctuation chars > where a punc char appears in wordchars. > > There should also be at least one test with both punctuation_chars and > whitespace_split=True. Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
