search

meticoeus / spring-icu

Provides the ICU4J's message formatting features, such as named arguments support, flexible plural formatting, rule based number format, date interval formats
Apache License 2.0
2 stars 4 forks source link

Result of resolveArguments() is unused in ICUMessageSourceSupport #2

Open eneveu opened 8 years ago

eneveu commented 8 years ago

In ICUMessageSourceSupport, I'm wondering if we should do

    synchronized (messageFormat) {
        return resolveArguments(args, locale).formatWith(messageFormat);
    }

instead of:

    resolveArguments(args, locale);
    synchronized (messageFormat) {
        return args.formatWith(messageFormat);
    }

That's what they do in the Spring MessageSourceSupport, and I don't understand what resolveArguments() does if we don't use the result?

https://github.com/meticoeus/spring-icu/blob/2ed67350712fa4fb81661fbdd6f0ceb85bd38e93/src/main/java/com/devtrigger/grails/icu/ICUMessageSourceSupport.java#L120-L123

https://github.com/spring-projects/spring-framework/blob/00d2606b000f9bdafbd7f4a16b6599fb51b53fa4/spring-context/src/main/java/org/springframework/context/support/MessageSourceSupport.java#L147-L149

meticoeus commented 8 years ago

I'm not exactly sure. I converted this from a Grails plugin I was using on a previous project so that it would work with just Spring. Most of the files in the project have not been modified, including this one.

I will take a look to see exactly what that would change. It does look strange.

meticoeus commented 8 years ago

It seems like the code was originally based on/copied from the Spring project.

Here is the commit where it was changed from what Spring does to the current implementation. There isn't much detail given as to why it changed.

It appears the args are not mutated by the method so it seems to be throwing away the result.