fix(util): add escape system so '*', '[]' can be escaped #236
Conversation
There was a problem hiding this comment.
Thank you very much for your PR!
The approach is good, but there are some cases that are not covered in your current implementation.
path.String() should also return the path with the escape characters (\) shown to avoid confusion.
I'm not sure exactly how we should do that. Maybe we can add a Original string field in the Path struct. This would break a ton of tests though.
Alternatively, we could keep the original path segment in the Name field and removeEscapeChars when walking in path.Walk. Other uses of this field (in the validation package) will need to be updated.
If you have a better suggestion, feel free to suggest!
| }, | ||
| }, path) | ||
|
|
||
| path, err = Parse("array[][].field\\[tx\\]") |
There was a problem hiding this comment.
This is great that this use-case works well! We should recommend the use of backquotes to avoid having to write double slashes, just like regexes.
| escape := map[rune]bool{ | ||
| '*': true, | ||
| '[': true, | ||
| ']': true, | ||
| } |
There was a problem hiding this comment.
Can we escape the dot (.) too?
| } | ||
|
|
||
| func TestEscapedParse(t *testing.T) { | ||
| path, err := Parse(`object\*field`) |
There was a problem hiding this comment.
* has an effect only if it's a full segment (e.g.: object.* <=> path.Name = "*"). Your test here is still correct and you should keep it. However, you should add one that makes sure it is possible to escape a "*" segment and that path.Walk() doesn't iterate on all keys in that case.
| @@ -436,6 +439,12 @@ func Depth(p string) uint { | |||
| } | |||
|
|
|||
| func createPathScanner(path string) *bufio.Scanner { | |||
| escape := map[rune]bool{ | |||
There was a problem hiding this comment.
For a tiny memory optimization, can you change this to a map[rune]struct{} please?
| // Skips the next character | ||
| i += width | ||
| continue | ||
| } | ||
| if isValidSyntax(r, next) { |
There was a problem hiding this comment.
You need to check in this function that a backslash (\) is followed by either another backslash or a character that can be escaped. You should also allow escaping the backslash character. path\to\element should be an invalid path, while path\\to\\element should be valid.
This will create a behavior that is more intuitive and closer to what developers are used to when escaping characters in general.
This function is badly named, it should be isSyntaxInvalid or something like that.
Pull Request Test Coverage Report for Build 12604780714Details
💛 - Coveralls |
References
Issue(s): #205
Description
Add escape system to enable
walk.Pathto use*[]in pathsExamples:
object\*fieldarray[][].field\\[]array[][].field\\[tx\\]