-
Notifications
You must be signed in to change notification settings - Fork 166
add support for balanced parens in query #120
base: master
Are you sure you want to change the base?
Conversation
add support for query containing and ending in balanced parens such as http://www.example.com?q=(1,2)
I'd like to add some test cases to the conformance tests as well http://www.example.com?q=(1,2) => http://www.example.com?q=(1,2) |
Oh sweet, thanks! |
The only issue i can think of with this fix is that I removed '(' and ')' from valid query characters, meaning that only balanced parens in query will be allowed. I'm not sure if this is an issue or not, but there are no unit tests that have parens in the query, other than the ones I added. |
'\\(' + | ||
'(?:' + | ||
'#{validUrlQueryChars}+' + | ||
'|' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you replace tab indentation with spaces in this line and alight plus signs on the right in this whole expression?
That sounds fair since we're already into balancing parens. |
@@ -304,7 +325,7 @@ | |||
'(#{validDomain})' + // $5 Domain(s) | |||
'(?::(#{validPortNumber}))?' + // $6 Port number (optional) | |||
'(\\/#{validUrlPath}*)?' + // $7 URL Path | |||
'(\\?#{validUrlQueryChars}*#{validUrlQueryEndingChars})?' + // $8 Query String | |||
'(\\?#{validUrlQuery})?' + // $8 Query String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here – align plus sign and the comment with the comments above, please.
|
add support for query containing and ending in balanced parens such as
http://www.example.com?q=(1,2)