-
-
Notifications
You must be signed in to change notification settings - Fork 151
Simplest fix for issue #169 (escaping separators in JavaPropsMapper). #332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Only works for single character separators and escapes. Only works with chars, not code points. Removes escape chars before separator chars, leaves other escapes alone. Sticks with the original processing until it encounters an escape before separator.
…recede the path separator, more testing.
Ok; I'll try to get this reviewed soon now, and assuming all goes well, there's just one other thing b efore merging: I need a CLA from: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf (or if you prefer, alternate Corporate CLA next to it). Looking forward to getting this merged in 2.14! |
properties/src/main/java/com/fasterxml/jackson/dataformat/javaprop/JavaPropsSchema.java
Show resolved
Hide resolved
properties/src/main/java/com/fasterxml/jackson/dataformat/javaprop/JavaPropsSchema.java
Outdated
Show resolved
Hide resolved
properties/src/main/java/com/fasterxml/jackson/dataformat/javaprop/JavaPropsSchema.java
Show resolved
Hide resolved
* Mutant factory method for constructing a new instance with | ||
* specified path separator escape; default being null ('\0') which | ||
* should effectively disable escape processing. | ||
* Any escape character may be used, backslash ('\\') is the most obvious candidate |
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.
Based on code I think this will only work for escaping path separator character itself (at least initial index check
verifies it is followed by path separator)
That is fine but should probably be explicitly mentioned so that users do not try to use it for escaping other characters. Might be also worth mentioning that JDK properties loader will handle Unicode escapes?
(a link to explanation would be great if one can be found).
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.
I've expanded the documentation on both the field and the method, making them both very similar.
I've also put the same content in the README.MD (hopefully I've got the markdown right).
properties/src/main/java/com/fasterxml/jackson/dataformat/javaprop/util/JPropPathSplitter.java
Outdated
Show resolved
Hide resolved
properties/src/main/java/com/fasterxml/jackson/dataformat/javaprop/util/JPropPathSplitter.java
Outdated
Show resolved
Hide resolved
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.
Looks good; added couple of notes (wrt adding @since
tags, suggestions for javadocs).
Mostly just need the CLA.
Excellent! I also got the CLA; hoping to do one more read-through and then get this all merged in. Thank you for working through all suggestions & for submitting the patch. |
This approach:
Only works for single character separators and escapes.
Only works with chars, not code points.
Removes escape chars before separator chars, leaves other escapes alone.
Sticks with the original processing until it encounters an escape before separator.
Fixes #169
I extended MapParsingTest to test the parsing, it's got a lot of backslashes in it because they need to be doubled twice (once for Java and once for properties file).