Add basic Validation for URITemplates #36
Conversation
|
Nice! That API certainly looks like it'd satisfy what |
| more, less = 'left', 'right' | ||
| if left_braces_count < right_braces_count: | ||
| more, less = less, more | ||
| super(UnbalancedBraces, self).__init__(uri, more, less) |
There was a problem hiding this comment.
Do you mean uri, more, less? The stuff you super for an exception will end up on e.args as the stuff that was passed to __init__, but the "user" of the exception here didn't pass those, they were intermediate values. It looks like you're using that to handle formatting them into your message, could always just define __str__ instead though obviously if you wanted to.
|
|
||
| def __init__(self): | ||
| """Initialize our validator.""" | ||
| self.enforcing_unbalanced_braces = True |
There was a problem hiding this comment.
Are you intentionally leaving this public? Will someone want to query one of these to find out whether it's enforcing things or not?
(Also could be a class attribute and ditch the method if you'd like)
There was a problem hiding this comment.
We don't document it, so instead of adding an underscore, I just left it public. If people want to try to mutate it, then oh well. Turning this into a property with a setter will mean that people might try to assign weird values and it breaks chaining.
| Returns the validator instance. | ||
| """ | ||
| self.enforcing_unbalanced_braces = False | ||
| return self |
There was a problem hiding this comment.
I take it you intentionally like chaining, even in cases where the instance itself has been mutated?
There was a problem hiding this comment.
I want to encourage chaining, yes. I've waffled back on having the instance be mutable or not, but in this case, I don't think it needs to be treated as immutable. I'm open to being convinced otherwise.
| :raises: uritemplate.exceptions.InvalidTemplate | ||
| :raises: uritemplate.exceptions.UnbalancedBraces | ||
| """ | ||
| if self.enforcing_unbalanced_braces: |
There was a problem hiding this comment.
Could get rid of this if if you wanted by instead having enforce_unbalanced_braces be a function rather than a boolean (and making it do nothing when you disable validation), although whether you'd do that probably depends heavily on your opinion above (about whether someone should want to write if somevalidator.enforcing_unbalanced_braces in external code bases)
There was a problem hiding this comment.
(about whether someone should want to write if somevalidator.enforcing_unbalanced_braces in external code bases)
I don't think it would hurt if someone wanted to print out the configuration in a log message for themselves, especially if their validation logic is somewhat dynamic.
|
|
||
|
|
||
| def _enforce_balanced_braces(template): | ||
| uri = template.uri |
There was a problem hiding this comment.
I still haven't read the spec unfortunately, so apologies, but is this actually enough? E.g., is }{}{}{ valid? Or do the braces need to be properly nested too?
There was a problem hiding this comment.
That is not valid, no. I'll have to work on some better validation for that situation.
Will probably change as python-hyper/uritemplate#36 is merged.
Will probably change as python-hyper/uritemplate#36 is merged.
|
@sigmavirus24 hey! Polite ping here, I think the only major issue here was about matching braces -- anything I can do to help there? |
|
Hi guys, there is no change for this PR get merged? |
Closes #33