Issue 11248043: Subscription validator (Closed)
|
Created:
10 years, 10 months ago by ana.balica Modified:
10 years, 10 months ago Reviewers:
thomas.j.waldmann, waldi Visibility:
Public. |
DescriptionSubscription validator
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Different splitting and test validation #
Total comments: 6
Patch Set 4 : Negative tests #
Total comments: 4
Patch Set 5 : Check validity of itemid subscription earlier #Patch Set 6 : Remove trailing whitespaces from tuples #Patch Set 7 : Remove trailing comma from tuples #
Total comments: 7
Patch Set 8 : Back to (el1, el2, ) style #
MessagesTotal messages: 13
https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... MoinMoin/storage/middleware/validation.py:327: if v is None: Can this happen? https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... MoinMoin/storage/middleware/validation.py:329: if v.startswith(keys.ITEMID): First check syntax, aka split it. Then check semantic. https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... MoinMoin/storage/middleware/validation.py:346: if namespace is not None: Above you already know that there is a namespace. Why do you postpone the check then? https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... MoinMoin/storage/middleware/validation.py:351: if not value: Why? Right now there is no item with empty name, but why should this be not allowed by syntax? https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... MoinMoin/storage/middleware/validation.py:364: element.add_error("The keyword value is not valid.") Shouldn't the other validators set proper errors on the element? Just copy them.
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... MoinMoin/storage/middleware/validation.py:351: if not value: On 2013/07/15 08:43:54, waldi wrote: > Why? Right now there is no item with empty name, but why should this be not > allowed by syntax? For instance you can set manually such a subscription: `namere::`. The namespace - default, the value is an empty string. Won't crash anything, but it doesn't make any sense to have it. https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... MoinMoin/storage/middleware/validation.py:364: element.add_error("The keyword value is not valid.") On 2013/07/15 08:43:54, waldi wrote: > Shouldn't the other validators set proper errors on the element? Just copy them. No, other validators are not setting any error messages. Just returning True or False.
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/1/MoinMoin/storage/middleware/va... MoinMoin/storage/middleware/validation.py:351: if not value: On 2013/07/15 09:57:59, ana.balica wrote: > On 2013/07/15 08:43:54, waldi wrote: > > Why? Right now there is no item with empty name, but why should this be not > > allowed by syntax? > For instance you can set manually such a subscription: `namere::`. > The namespace - default, the value is an empty string. Won't crash anything, but > it doesn't make any sense to have it. An empty regex is valid. It would even match everything. https://codereview.appspot.com/11248043/diff/6001/MoinMoin/storage/middleware... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/6001/MoinMoin/storage/middleware... MoinMoin/storage/middleware/validation.py:328: if len(subscription) == 2: The name can contain ":", so you need to split at most two times. You want to do something like this: keyword, value = element.value.split(':', 1) if keyword in (keys.ITEMID, ): pass elif keyword in (…): namespace, value = value.split(':', 1) check namespace else: error https://codereview.appspot.com/11248043/diff/6001/MoinMoin/storage/middleware... MoinMoin/storage/middleware/validation.py:336: if keyword not in [keys.NAME, keys.TAGS, keys.NAMERE, keys.NAMEPREFIX, ]: Never use lists if a tuple is sufficient. Check PEP-8 how this should look.
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:328: except ValueError: For cases when there is no colon at all. https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:336: except ValueError: Use case when there is a single colon and the keyword is not <itemid>.
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/_tests/test_validation.py (right): https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/_tests/test_validation.py:74: u"{0}:userprofiles:a".format(keys.NAMEPREFIX)] Where are the negative tests? https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:333: elif keyword in (keys.NAME, keys.TAGS, keys.NAMERE, keys.NAMEPREFIX, ): Please actually read PEP-8! https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:348: value_element = String(value) Please add some empty lines.
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/10001/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:333: elif keyword in (keys.NAME, keys.TAGS, keys.NAMERE, keys.NAMEPREFIX, ): On 2013/07/15 13:37:08, waldi wrote: > Please actually read PEP-8! I read http://www.python.org/dev/peps/pep-0008/ and no specific guides on how to check if a value is in an iterable, tuple formatting, list vs tuple usages. My best guess is to use a set instead of a list or tuple. if keyword in {keys.NAME, }. But didn't find anything on that too. What should it look like?
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/8002/MoinMoin/storage/middleware... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/8002/MoinMoin/storage/middleware... MoinMoin/storage/middleware/validation.py:334: elif keyword in (keys.NAME, keys.TAGS, keys.NAMERE, keys.NAMEPREFIX, ): | Avoid extraneous whitespace in the following situations: | Immediately inside parentheses, brackets or braces. [http://www.python.org/dev/peps/pep-0008/#pet-peeves] https://codereview.appspot.com/11248043/diff/8002/MoinMoin/storage/middleware... MoinMoin/storage/middleware/validation.py:352: value_is_valid = uuid_validator(value_element, state) You could merge this in the if-clause above. But do it in a separate change.
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/8002/MoinMoin/storage/middleware... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/8002/MoinMoin/storage/middleware... MoinMoin/storage/middleware/validation.py:334: elif keyword in (keys.NAME, keys.TAGS, keys.NAMERE, keys.NAMEPREFIX, ): On 2013/07/15 15:43:37, waldi wrote: > | Avoid extraneous whitespace in the following situations: > | Immediately inside parentheses, brackets or braces. > [http://www.python.org/dev/peps/pep-0008/#pet-peeves] How about one-value tuples? Based on your example it is: if keyword in (keys.ITEMID, ): Should it become (keys.ITEMID,)?
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/8002/MoinMoin/storage/middleware... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/8002/MoinMoin/storage/middleware... MoinMoin/storage/middleware/validation.py:334: elif keyword in (keys.NAME, keys.TAGS, keys.NAMERE, keys.NAMEPREFIX, ): On 2013/07/15 15:54:52, ana.balica wrote: > How about one-value tuples? Based on your example it is: > if keyword in (keys.ITEMID, ): > Should it become (keys.ITEMID,)? No, there should be a space before the ")". https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:334: value_is_valid = uuid_validator(value_element, state) I said: do that in a separate change.
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:332: if keyword in (keys.ITEMID,): pep8 wants a blank after the comma https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:335: elif keyword in (keys.NAME, keys.TAGS, keys.NAMERE, keys.NAMEPREFIX): here I'ld also like the trailing comma (and the blank after it) https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:353: value_is_valid = tag_validator(value_element, state) maybe just "valid" instead "value_is_valid" is also pretty understandable. https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:361: return True merge these 2 lines as "return valid" ?
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:335: elif keyword in (keys.NAME, keys.TAGS, keys.NAMERE, keys.NAMEPREFIX): On 2013/07/16 16:19:26, Thomas.J.Waldmann wrote: > here I'ld also like the trailing comma (and the blank after it) Earlier I was suggested to change this https://codereview.appspot.com/11248043/patch/19001/20002 to the no blanks, no commas. We all need to agree on a single style.
Sign in to reply to this message.
https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/validation.py (right): https://codereview.appspot.com/11248043/diff/23002/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/validation.py:334: value_is_valid = uuid_validator(value_element, state) On 2013/07/16 16:07:45, waldi wrote: > I said: do that in a separate change. What does mean then a separate change? New code review? Patch? Commit? Please be more specific. I have fixed it in a separate patch: #5 https://codereview.appspot.com/11248043/diff2/8002:19001/MoinMoin/storage/mid...
Sign in to reply to this message.
The validator is pushed on my public repo. https://bitbucket.org/ana-balica/moin-2.0/commits/d4f0bb40c38deec0cc010bf1ac6...
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
