Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1606)

Issue 334320043: ticket:13526 Refactoring MeasureFormat, CurrencyFormat, and TimeUnitFormat to more directly wrap Nu… (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months, 3 weeks ago by sffc
Modified:
10 months, 1 week ago
Reviewers:
markus.icu
CC:
andy.heninger
Base URL:
svn+icussh://source.icu-project.org/repos/icu/trunk/
Visibility:
Public.

Description

ticket:13526 Refactoring MeasureFormat, CurrencyFormat, and TimeUnitFormat to more directly wrap NumberFormatter.

Patch Set 1 #

Patch Set 2 : Rebasing after svn update. #

Total comments: 21

Patch Set 3 : Rebasing again against a revision with auto-formatter applied on the original file. #

Patch Set 4 : Markus Round 1 #

Total comments: 2

Patch Set 5 : Changing StringBuilder -> Appendable internally #

Patch Set 6 : All tests passing (including serialization) #

Total comments: 1

Messages

Total messages: 17
sffc
This rietveld is my initial attempt at refactoring MeasureFormat to wrap NumberFormatter. Most unit tests ...
11 months, 3 weeks ago (2017-12-23 03:55:40 UTC) #1
sffc
Rebasing after svn update.
11 months ago (2018-01-08 21:26:04 UTC) #2
sffc
11 months ago (2018-01-08 21:50:44 UTC) #3
sffc
11 months ago (2018-01-08 21:51:05 UTC) #4
markus.icu
In the future please don't mix auto-formatter changes and functional changes. https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java File icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java (right): ...
11 months ago (2018-01-10 22:00:13 UTC) #5
sffc
Rebasing again against a revision with auto-formatter applied on the original file.
11 months ago (2018-01-11 00:53:51 UTC) #6
sffc
Markus Round 1
11 months ago (2018-01-11 05:33:04 UTC) #7
sffc
Markus Round 1
11 months ago (2018-01-11 05:34:06 UTC) #8
sffc
Addressed first round of comments. Round-trip serialization is still not completely working. I'm not going ...
11 months ago (2018-01-11 05:35:59 UTC) #9
markus.icu
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java File icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java (right): https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java#newcode343 icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:343: // /** Ok. Please check that CLDR does not ...
11 months ago (2018-01-11 21:03:25 UTC) #10
sffc
Changing StringBuilder -> Appendable internally
11 months ago (2018-01-13 06:21:25 UTC) #11
sffc
Working on the serialization tonight. https://codereview.appspot.com/334320043/diff/80001/icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java File icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java (right): https://codereview.appspot.com/334320043/diff/80001/icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java#newcode311 icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:311: // Can't directly use ...
11 months ago (2018-01-13 06:25:21 UTC) #12
sffc
All tests passing (including serialization)
11 months ago (2018-01-13 08:55:31 UTC) #13
sffc
Serialization is working, and it's even backwards-compatible thanks to decisions made by previous engineers who've ...
11 months ago (2018-01-13 08:57:46 UTC) #14
sffc
Ping Markus Once you give it the okay, I will check in this change and ...
10 months, 3 weeks ago (2018-01-19 23:35:41 UTC) #15
markus.icu
LGTM except one typo https://codereview.appspot.com/334320043/diff/120001/icu4j/main/classes/core/src/com/ibm/icu/text/CurrencyFormat.java File icu4j/main/classes/core/src/com/ibm/icu/text/CurrencyFormat.java (right): https://codereview.appspot.com/334320043/diff/120001/icu4j/main/classes/core/src/com/ibm/icu/text/CurrencyFormat.java#newcode70 icu4j/main/classes/core/src/com/ibm/icu/text/CurrencyFormat.java:70: // Preserve backward serialize backward ...
10 months, 1 week ago (2018-02-03 00:25:56 UTC) #16
sffc
10 months, 1 week ago (2018-02-03 03:44:59 UTC) #17
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b