fixed bug where dot notation doesn't work properly #72
Conversation
Dot notation doesn't work when the parent is a merged object. This should fix the issue.
converting path to string did not perform proper escaping. this fixes the issue.
src/HOCON/Impl/HoconPath.cs
Outdated
| foreach (string subPath in this) | ||
| { | ||
| // #todo escape newline chars in path? | ||
| if (subPath.NeedTripleQuotes()) |
There was a problem hiding this comment.
This isn't needed, triple quotes in paths are rejected during creation, as they are illegal by HOCON specification.
There was a problem hiding this comment.
Sorry, my code did not illustrate my concern properly. The problem I have is that a key is technically allowed to contain \n or other control characters. This creates a problem when you need to reference it in a path, as we can't triple quote it. I guess the solution is to escape all the offending characters automatically, which I was too lazy to implement.
To be clear, my code above does not fully address the concern above. Any thoughts?
There was a problem hiding this comment.
well, triple quotes in field keys are not allowed in the HOCON specification, and it is enforced when the key is created/parsed, so there is no need to check for a triple quote there.
src/HOCON/Impl/Utils.cs
Outdated
| @@ -124,6 +124,17 @@ public static bool Contains(this string s, char c) | |||
| return s.IndexOf(c) != -1; | |||
| } | |||
|
|
|||
| public static bool ContainsHoconWhitespaceExceptNewLine(this string s) | |||
There was a problem hiding this comment.
This is is covered by the NeedTripleQuotes() string extension.
There was a problem hiding this comment.
Again, I was worried that a key may contain whitespace (such as my key). NeedTripleQuotes calls NeedQuotes, which I understand does not check for whitespace. Did not cross my mind, but would a path such as foo.my key.bar be valid (or does it have to be foo."my key".bar)?
There was a problem hiding this comment.
foo."my.key".bar and foo."my long spacious key".bar are both legal, but not foo."""my\nkey""".bar. Your concern is correct, there is a bug in Path parsing. My point is that, instead of making another extension method, you should fix the NeedTripleQuotes() method instead
src/HOCON/Impl/Utils.cs
Outdated
| @@ -182,5 +193,19 @@ public static bool NeedQuotes(this string s) | |||
| public static bool NeedTripleQuotes(this string s) | |||
| => s.NeedQuotes() && s.Contains(Utils.NewLine); | |||
|
|
|||
| public static string AddQuotesIfRequired(this string s) | |||
There was a problem hiding this comment.
You can use ToHoconSafe() string extension instead.
There was a problem hiding this comment.
You are right. I was concerned that a key may contain the \ character. Would that be a problem?
There was a problem hiding this comment.
It shouldn't be a problem, unless you can found a case where there is a problem? If there is a problem, then ToHoconSafe() should be fixed too.
- less modification on existing extensions - support for converting keys with special values to path: `\n`, (empty), `"`, `.` and `\`.
|
hi i made some modifications to my pr based on your comments :) |
|
Looking good. We'll try to improve the backslash substitution problem later, your solution is good for now 👍 |
This is an attempt at addressing issue #71
The bug is caused by attempting to add a new key to a merged object, which is not the same as the field's actual underlying value.