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

Introducing a Messages.formatWithAttributes() method + unit tests #1649

Merged
merged 12 commits into from Jan 30, 2018

Conversation

Projects
None yet
3 participants
@andreiruse
Member

andreiruse commented Jan 26, 2018

This new method will not only return the formatted message with values instead of placeholders but also a map of attributes.

Messages.format("You have included {} placeholders, however only provided {} arguments.", matchesCount, args.length));
}
matcher.reset(); //Since we have already called .find(), and that state needs to be reset
while (matcher.find()) {

This comment has been minimized.

@@ -110,4 +118,74 @@ public static String format(String messageTemplate, Object... args) {
return builder.toString();
}
/**
* Formats a templated message inserting named arguments.

This comment has been minimized.

@cjkent

cjkent Jan 26, 2018

Member

I don't think these Javadoc make it clear enough what the method does. An example would be worth a thousand words.

while (matcher.find()) {
matchesCount++;
}
if (matchesCount > args.length) {

This comment has been minimized.

@cjkent

cjkent Jan 26, 2018

Member

Could you roll this check into the replacement loop?

*/
public static Pair<String, Map<String, String>> formatWithAttributes(String messageTemplate, Object... args) {
if (messageTemplate == null) {
return formatWithAttributes("", args);

This comment has been minimized.

@cjkent

cjkent Jan 26, 2018

Member

Would this achieve the same thing?

return "";

This comment has been minimized.

@andreiruse

andreiruse Jan 26, 2018

Member

Logically, yes. However, the method has to return Pair<String, Map<String, String>> - returning a String would not work.

This comment has been minimized.

@cjkent

cjkent Jan 26, 2018

Member

My point was that it's probably clearer to return immediately rather than calling back into the method and asking the reader to work out how that would behave. Like this:

return Pair.of("", ImmutableMap.of());
return formatWithAttributes("", args);
}
if (args == null) {
return formatWithAttributes(messageTemplate, new Object[0]);

This comment has been minimized.

@cjkent

cjkent Jan 26, 2018

Member

Is this the same as:

return messageTemplate;

This comment has been minimized.

@andreiruse

andreiruse Jan 26, 2018

Member

Logically, yes. However, the method has to return Pair<String, Map<String, String>> - returning a String would not work

ImmutableMap.Builder<String, String> attributesMap = ImmutableMap.builder();
String pattern = "\\{(\\w+)\\}";
Pattern regexPattern = Pattern.compile(pattern);

This comment has been minimized.

@cjkent

cjkent Jan 26, 2018

Member

Compiling a pattern is relatively expensive. Do it once and put it in a static final field. Pattern is thread safe and can be shared.

String pattern = "\\{(\\w+)\\}";
Pattern regexPattern = Pattern.compile(pattern);
Matcher matcher = regexPattern.matcher(messageTemplate);

This comment has been minimized.

@cjkent

cjkent Jan 26, 2018

Member

Using a regex might be a performance problem. But manually parsing is likely to be fiddly. I'd use a regex for simplicity until it's obviously a problem.

return new Object[][]{
// null template
{null, null, Pair.of("", ImmutableMap.of())},
{null, new Object[]{}, Pair.of("", ImmutableMap.of())},

This comment has been minimized.

@jodastephen

jodastephen Jan 26, 2018

Member

There should be one space between the square and curly braces [] { on each of these lines

* The message template contains zero to many "{name}" placeholders.
* Each placeholder is replaced by the next available argument.
* Empty "{}" placeholders do not get replaced, and will be present in the output in the same form.
* If there are too few arguments, then an {@link IllegalArgumentException} will be thrown.

This comment has been minimized.

@jodastephen

jodastephen Jan 26, 2018

Member

No exceptions. This method is used when producing exceptions, so it must not throw one.

* <p>
* The message template contains zero to many "{name}" placeholders.
* Each placeholder is replaced by the next available argument.
* Empty "{}" placeholders do not get replaced, and will be present in the output in the same form.

This comment has been minimized.

@jodastephen

jodastephen Jan 26, 2018

Member

No. {} is still a placeholder, just an unamed one.

This comment has been minimized.

@andreiruse

andreiruse Jan 26, 2018

Member

That argument is outdated. Both named and unnamed placeholders are not replaced, as expected

// try to make builder big enough for the message and the args
StringBuilder outputStringBuilder = new StringBuilder(messageTemplate.length() + args.length * 20);
Map<String, String> attributesMap = new HashMap<>();

This comment has been minimized.

@jodastephen

jodastephen Jan 26, 2018

Member

Add a comment to not use ImmutableMap as duplicate keys must be allowed even though there is data loss (to avoid exceptions)

int groupIndex = 0;
int lastAddedStringEndIndex = 0;
int matchesCount = 0;
while (matcher.find()) {

This comment has been minimized.

@jodastephen

jodastephen Jan 26, 2018

Member

This loop won't be needed once you get rid of the exception

andreiruse added some commits Jan 26, 2018

if (matchesCount > args.length) {
throw new IllegalArgumentException(

This comment has been minimized.

@cjkent

cjkent Jan 26, 2018

Member

Don't throw an exception. Just ignore the extra placeholders so they appear in the output as {} or {foo}.

andreiruse added some commits Jan 26, 2018

@@ -110,4 +120,57 @@ public static String format(String messageTemplate, Object... args) {
return builder.toString();
}
/**
* Formats a templated message inserting named arguments. Typical template would look like "Hello, {attributeName}".

This comment has been minimized.

@cjkent

cjkent Jan 26, 2018

Member

I still think this needs an example. I don't think it's clear from the docs what it does. Something like:

Messages.formatWithAttributes("Foo={foo}, Bar={}", "abc", 123);

returns the message

"Foo=abc, Bar=123" 

and the map

{"foo": "123"}

Feel free to copy and paste :)

andreiruse added some commits Jan 30, 2018

@cjkent

cjkent approved these changes Jan 30, 2018

@andreiruse andreiruse merged commit dedab3e into master Jan 30, 2018

15 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk No dependency changes
Details
security/snyk - modules/basics/pom.xml No dependency changes
Details
security/snyk - modules/calc/pom.xml No dependency changes
Details
security/snyk - modules/collect/pom.xml No dependency changes
Details
security/snyk - modules/data/pom.xml No dependency changes
Details
security/snyk - modules/loader/pom.xml No dependency changes
Details
security/snyk - modules/market/pom.xml No dependency changes
Details
security/snyk - modules/math/pom.xml No dependency changes
Details
security/snyk - modules/measure/pom.xml No dependency changes
Details
security/snyk - modules/pom.xml No dependency changes
Details
security/snyk - modules/pricer/pom.xml No dependency changes
Details
security/snyk - modules/product/pom.xml No dependency changes
Details
security/snyk - modules/report/pom.xml No dependency changes
Details

@andreiruse andreiruse deleted the topic/messages-format-with-attributes branch Jan 30, 2018

@jodastephen jodastephen added this to the v1.7 milestone Feb 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment