Skip to content

Conversation

@bmarwell
Copy link
Contributor

@bmarwell bmarwell commented Apr 1, 2020

…f values

  • Do not skip escape characters for the value (new behaviour demanded by SHIRO-530).
  • rearrange and comment tests to reflect old and new, desired behaviour.

Signed-off-by: Benjamin Marwell bmarwell@gmail.com

Alternative to #209

…f values

 - Do not skip escape characters for the value (new behaviour demanded by SHIRO-530).
 - rearrange and comment tests to reflect old and new, desired behaviour.

Signed-off-by: Benjamin Marwell <bmarwell@gmail.com>
@fpapon fpapon self-requested a review April 1, 2020 09:12
@fpapon
Copy link
Member

fpapon commented Apr 1, 2020

@bmhm sounds good!

Copy link
Member

@fpapon fpapon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fpapon
Copy link
Member

fpapon commented Apr 8, 2020

@bdemers can you review please just before I merge it?

@fpapon
Copy link
Member

fpapon commented Apr 9, 2020

retest this please


test = "Truth=Beau\\ty";
// SHIRO-530: Keep backslashes in value.
test = "Truth=Beauty\\";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this one wrap? and the next line would be included?
i.e. something like:

Truth=Beauty\
more text here

Which would be something like Truth=Beatuty\\\nmore text here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it should not, because we are calling the splitKeyValue method. The backslashes at the end are resolved in the caller method. When arriving at this point, the wrapping would have already occured. Took me a while, too, to figure this one out. But I did not bother to add more comments. ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdemers see here:

private static Map<String, String> toMapProps(String content) {
Map<String, String> props = new LinkedHashMap<String, String>();
String line;
StringBuilder lineBuffer = new StringBuilder();
Scanner scanner = new Scanner(content);
while (scanner.hasNextLine()) {
line = StringUtils.clean(scanner.nextLine());
if (isContinued(line)) {
//strip off the last continuation backslash:
line = line.substring(0, line.length() - 1);
lineBuffer.append(line);
continue;
} else {
lineBuffer.append(line);
}
line = lineBuffer.toString();
lineBuffer = new StringBuilder();
String[] kvPair = splitKeyValue(line);
props.put(kvPair[0], kvPair[1]);
}
return props;
}

Please check if I did not mis-read it.

Copy link
Member

@bdemers bdemers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question in there (the backslashes are making my brain hurt)

@fpapon
Copy link
Member

fpapon commented Apr 9, 2020

I think the complexity is that we can have some regex or URI in the value, so we have to deal correctly with the backslash

Copy link
Member

@bdemers bdemers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmhm Thanks for the link, that resolves the open question I had

Great work!

@fpapon
Copy link
Member

fpapon commented Apr 10, 2020

@bmhm Thanks for the great work ;)

@fpapon fpapon merged commit 8b46c8a into apache:master Apr 10, 2020
@bmarwell bmarwell deleted the SHIRO-530-alt branch May 11, 2020 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants