Update to DateSerializer and new LocaleSerializer #164
Conversation
serverperformance
commented
Jan 5, 2014
serverperformance
commented
- Updated DateSerializer to properly handle subclasses (java.sql.Date, Time, Timestamp, etc).
- New LocaleSerializer default immuable serializer
- DateSerializer now properly handles Date subclasses. - Added a new default serialzer por java.util.Locale.
Update to DateSerializer and new LocaleSerializer
|
A little strange to sort of bundle multiple types in one serializer, but I see why you did it. Probably could use == for class comparison, but makes little difference. Thanks for contributing! |
|
No tests? |
|
Sorry, you are right. I need time to undertand the rounTrip structure in the current tests -----Original Message----- No tests? Reply to this email directly or view it on GitHub: |
|
For j.u.Date subclasses they should be similar to the Date test, for LocaleSerializer you could adapt the test from my PR. |
|
Ok! Thank you... But not tonight, here in Spain and other latin countries, this is the night in which the Three Kings ("Los Reyes Magos") bring the gifts to our children, so shhh I must go to sleep soon ;) -----Original Message----- For j.u.Date subclasses they should be similar to the Date test, for LocaleSerializer you could adapt the test from my PR #100. Reply to this email directly or view it on GitHub: |
|
Looking at it deeper while creating the unit tests: java.sql.Timestamp needs a separate default serializer as it handles nanos (from the javadoc: "Due to the differences between the Timestamp class and the java.util.Date class mentioned above, it is recommended that code not view Timestamp values generically as an instance of java.util.Date. The inheritance relationship between Timestamp and java.util.Date really denotes implementation inheritance, and not type inheritance.") The other subclasses, at least j.sql.Date and j.sql.Time, can actually be ahndled as I proposed. What do you think? Two separate default serializers seems ok to you? On the other hand I will update LocaleSerializer to remove the copy method, as it is immutable. I will make tonight a pull request with these changes except if you propose a better alternative for Date subclasses... Cheers, -----Original Message----- Ok! Thank you... But not tonight, here in Spain and other latin countries, this is the night in which the Three Kings ("Los Reyes Magos") bring the gifts to our children, so shhh I must go to sleep soon ;) -----Original Message----- For j.u.Date subclasses they should be similar to the Date test, for LocaleSerializer you could adapt the test from my PR #100. Reply to this email directly or view it on GitHub: |
|
By the way: DateSerializer optimizes writeLong assuming that the values are only positive... Which is the common case I supose but not mandatory in fact. In the java.sql.Date and java.sql.Timestamp at least this optimization can't be done. AND it it wouldn't be for backwards compatibility I would re-think that optimization in the base DateSerializar... Umm -----Original Message----- Looking at it deeper while creating the unit tests: java.sql.Timestamp needs a separate default serializer as it handles nanos (from the javadoc: "Due to the differences between the Timestamp class and the java.util.Date class mentioned above, it is recommended that code not view Timestamp values generically as an instance of java.util.Date. The inheritance relationship between Timestamp and java.util.Date really denotes implementation inheritance, and not type inheritance.") The other subclasses, at least j.sql.Date and j.sql.Time, can actually be ahndled as I proposed. What do you think? Two separate default serializers seems ok to you? On the other hand I will update LocaleSerializer to remove the copy method, as it is immutable. I will make tonight a pull request with these changes except if you propose a better alternative for Date subclasses... Cheers, -----Original Message----- Ok! Thank you... But not tonight, here in Spain and other latin countries, this is the night in which the Three Kings ("Los Reyes Magos") bring the gifts to our children, so shhh I must go to sleep soon ;) -----Original Message----- For j.u.Date subclasses they should be similar to the Date test, for LocaleSerializer you could adapt the test from my PR #100. Reply to this email directly or view it on GitHub: |
|
I just made another pull request. Forget my previous messages with my doubts. Unit tests work ok, and a deep review of the j.sql.Timestamp, etc, source code cleared all my doubts. Sorry. |
