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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 3 days ago by sffc
Modified:
3 days, 2 hours 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) #

Messages

Total messages: 14
sffc
This rietveld is my initial attempt at refactoring MeasureFormat to wrap NumberFormatter. Most unit tests ...
3 weeks, 3 days ago (2017-12-23 03:55:40 UTC) #1
sffc
Rebasing after svn update.
1 week ago (2018-01-08 21:26:04 UTC) #2
sffc
1 week ago (2018-01-08 21:50:44 UTC) #3
sffc
1 week 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): ...
5 days, 13 hours ago (2018-01-10 22:00:13 UTC) #5
sffc
Rebasing again against a revision with auto-formatter applied on the original file.
5 days, 10 hours ago (2018-01-11 00:53:51 UTC) #6
sffc
Markus Round 1
5 days, 6 hours ago (2018-01-11 05:33:04 UTC) #7
sffc
Markus Round 1
5 days, 6 hours 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 ...
5 days, 6 hours 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 ...
4 days, 14 hours ago (2018-01-11 21:03:25 UTC) #10
sffc
Changing StringBuilder -> Appendable internally
3 days, 5 hours 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 ...
3 days, 5 hours ago (2018-01-13 06:25:21 UTC) #12
sffc
All tests passing (including serialization)
3 days, 2 hours ago (2018-01-13 08:55:31 UTC) #13
sffc
3 days, 2 hours ago (2018-01-13 08:57:46 UTC) #14
Serialization is working, and it's even backwards-compatible thanks to decisions
made by previous engineers who've worked on this class.  To fix the tests, it
turns out all I needed to to was add back a bit of code I had deleted from
CurrencyFormat, and then in the writeReplace methods, call getNumberFormat()
instead of trying to access the numberFormat private field, which is not used by
TimeUnitFormat.
Sign in to reply to this message.

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