#3628 Add locale parameter for numberFormat by Obolrom · Pull Request #3693 · mapstruct/mapstruct

Obolrom · 2024-09-03T09:57:26Z

Implementation of enhancement #3628

add Mapping#locale param
adjust decimal format creation logic
add test case for other default lang
support locale param for Mapping#dateFormat
integrate locale param for MapMapping & IterableMapping annotations
add documentation for locale params

e.g.

@Mapping( target = "bigDecimal1", numberFormat = "#0.#E0", locale = "en/US" )

Fixes #3628

filiphr

Thanks for your work on this @Obolrom. It is on the right track. However, it still needs some work before it is in a mergeable state. I've left some inline comments, but there are some other more general remarks as well:

The use of new Locale public constructor. This constructor has been deprecated in Java 19 and there is Locale.forLanguageTag available since Java 7, let's use that everywhere instead.
I see that we are only supporting the numberFormat in the @Mapping for this. However, we also have dateFormat in @Mapping and we also have the following (if we are adding the locale we need to support it everywhere):
- MapMapping#keyNumberFormat
- MapMapping#keyDateFormat
- MapMapping#valueNumberFormat
- MapMapping#valueDateFormat
- IterableMapping#numberFormat
- IterableMapping#dateFormat
I don't see any tests with a custom locale. The NumberFormatConversionRuLocaleTest is identical to NumberFormatConversionTest, with the only difference being that a different default locale is used. What we need instead is in the NumberFormatConversionTest (which runs using the default en locale to have mappers using a custom locale.
An additional improvement could be that we store the passed Locale to the method in a field, but this is more complex and this can be an additional improvement.

core/src/main/java/org/mapstruct/Mapping.java

processor/src/main/java/org/mapstruct/ap/internal/model/HelperMethod.java

processor/src/main/resources/org/mapstruct/ap/internal/conversion/CreateDecimalFormat.ftl

Obolrom · 2024-09-04T12:07:47Z

@filiphr, thank you for the quick feedback.
I have integrated locale param for IterableMapping & MapMapping, added documentation, fixed tests, and made the code a little cleaner. So it is ready for review.

However, I didn't understand your idea about

An additional improvement could be that we store the passed Locale to the method in a field, but this is more complex and this can be an additional improvement.

filiphr · 2024-09-13T23:05:18Z

However, I didn't understand your idea about

Ignore that for now. It isn't that important

filiphr

I've missed some things in my previous review. Have a look at my comments. We strive to have a readable generated code. What is being done now is not really readable, let's try to not have full qualified names if possible.

filiphr · 2024-09-13T23:09:04Z

processor/src/main/java/org/mapstruct/ap/internal/conversion/BigDecimalToStringConversion.java

+        if ( conversionContext.getLocale() != null ) {
+            sb.append( ", java.util.Locale.forLanguageTag( \"" );
+            sb.append( conversionContext.getLocale() );
+            sb.append( "\" )" );
+        }
+        else {
+            sb.append( ", java.util.Locale.getDefault() " );
+        }


We strive to have as much readable code as possible. having a full qualified name here isn't readable 😉. Please have a look at how we are doing it in the getFromExpression for the BigDecimal import

filiphr · 2024-09-13T23:09:24Z

processor/src/main/java/org/mapstruct/ap/internal/conversion/BigIntegerToStringConversion.java

+        if ( conversionContext.getLocale() != null ) {
+            sb.append( ", Locale.forLanguageTag( \"" );
+            sb.append( conversionContext.getLocale() );
+            sb.append( "\" )" );
+        }
+        else {
+            sb.append( ", Locale.getDefault() " );
+        }


Same comment as in BigDecimalToStringConversion

Obolrom · 2024-09-14T09:49:32Z

@filiphr
I have fixed the issues you mentioned in the last comment.

However, right now we have some code duplications
WrapperToStringConversion::appendDecimalFormatter is identical to PrimitiveToStringConversion::appendDecimalFormatter
and
BigDecimalToStringConversion::appendDecimalFormatter is identical to BigIntegerToStringConversion::appendDecimalFormatter.

We can make appendDecimalFormatter method abstract and move it to AbstractNumberToStringConversion.
However, in this case, the code will still be duplicated.
What do you think of that?

…otation

…notation

Obolrom · 2024-11-03T10:26:32Z

@filiphr
Kind reminder to review PR after changes 😄

filiphr · 2024-11-03T14:30:55Z

Looks better @Obolrom. However, I see that there is a change in behaviour and now we always pass Locale.getDefault to certain places. I would say that we should only pass some locale if there is something set by the user. Otherwise, we keep doing what we were doing before.

i.e. there should be no need to change anything in existing tests.

filiphr · 2024-11-03T14:31:18Z

If you are busy, I can apply these changes while merging

Obolrom · 2024-11-03T14:32:49Z

@filiphr
It would be great if we close this PR today.
Feel free to apply these changes

filiphr · 2024-11-03T14:38:21Z

@Obolrom it will most definitely not be closed today. I'll apply the changes when I start the merge

Obolrom marked this pull request as ready for review September 3, 2024 12:55

Obolrom mentioned this pull request Sep 3, 2024

Add locale parameter for numberFormat #3628

Closed

filiphr requested changes Sep 3, 2024

View reviewed changes

Obolrom requested a review from filiphr September 4, 2024 12:02

filiphr requested changes Sep 13, 2024

View reviewed changes

Obolrom requested a review from filiphr September 14, 2024 09:49

Obolrom force-pushed the 3628_number_format_locale_param branch from 1580228 to 2d461c8 Compare September 15, 2024 18:31

Obolrom added 18 commits September 24, 2024 13:27

mapstruct#3628 Add locale parameter for numberFormat

2e497d5

mapstruct#3628 Fix code style

083f031

mapstruct#3628 Add naive implementation of locale param

2fffb30

mapstruct#3628 Fix code style

0a69ef5

mapstruct#3628 Use decimal format constructor with 2 params

c9a4714

mapstruct#3628 Remove getLocaleParameter in HelperMethod

dbb22b7

mapstruct#3628 Adjust test cases to use custom locale

7031e5d

mapstruct#3628 Implemented date format support locale for Mapping ann…

4850e44

…otation

mapstruct#3628 Fix code style

eb20595

mapstruct#3628 Fix unused import error

ffa8992

mapstruct#3628 Add locale support for MapMapping & IterableMapping an…

d32e9f6

…notation

mapstruct#3628 Fix imports

ffc969f

mapstruct#3628 Fix java 21 fixture

6ef61d4

mapstruct#3628 Fix fixture for java 21

1b01df1

mapstruct#3628 Adjust documentation for locale param

78993d9

mapstruct#3628 Refactor imports for generated code

6e1f4af

mapstruct#3628 Fixed code style

d74f87c

mapstruct#3628 Fixed code style

7388038

Obolrom force-pushed the 3628_number_format_locale_param branch from 2d461c8 to 7388038 Compare September 24, 2024 10:29

Polish

16951e8

filiphr merged commit 737af6b into mapstruct:main Nov 17, 2024

Uh oh!

Conversation

Obolrom commented Sep 3, 2024 • edited by filiphr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Obolrom commented Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filiphr commented Sep 13, 2024

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

filiphr Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

filiphr Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

Obolrom commented Sep 14, 2024

Uh oh!

Obolrom commented Nov 3, 2024

Uh oh!

filiphr commented Nov 3, 2024

Uh oh!

filiphr commented Nov 3, 2024

Uh oh!

Obolrom commented Nov 3, 2024

Uh oh!

filiphr commented Nov 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Obolrom commented Sep 3, 2024 •

edited by filiphr

Loading

Obolrom commented Sep 4, 2024 •

edited

Loading