On 04/12/2013 05:55 AM, Alan Bateman wrote:
Sherman and I chatted about adding from(Instant)/toInstant() a few times over
the last week so most of my comments are already included.
Overall I think it has worked out well, just unfortunate that there is a bit of
extra complexity because FileTime supports a wider time-line and we also went
with the XML schema deviation from (older?) ISO 8601 for year 0000. In
practical terms these aren't of course interesting for file times.
In terms of API then from(Instant)/toInstant() are simple and I don't think we
have an urgent need for FileTimes to be directly formatted.
On toString then FileTime doesn't require that trailing zeros be stripped from
the fraction of a second. If it is better to use 3-digit units that it is okay.
Also if this is something that we got wrong in FileTime then we could change
the spec without any compatibility impact.
I'm not sure if the 3-digit unit fitting is "better" or not. But xml spec cited
in the
toString() API specifies that the "trailing zeros are optional" in its 3.2.3.1
"lexical
representatin" section and "trailing zeros are prohibited..." in 3.2.3.2
"canonical
representation" section. So I think trimming trailing zeros in
FileTime.toString()
is just fine.
http://www.w3.org/TR/xmlschema-2/#deviantformats
Values outside of MIN/MAX aren't too interesting so hashCode using Instant
hashCode should be fine. I had to read compareTo a few times to convince myself
that it correctly orders FileTimes where one is created from an Instant and the
other from a value outside of MIN/MAX. It might be useful to expand the comment
to explain this further.
Added some wording.
Thanks for expanding the existing test. A minor point at line 117 is that you don't need
"Random r" as there is rand already. The pre-existing tests loop over
TimeUnit.values() rather than EnumSet.allOf(TimeUnit.class). In eqTime then it prints to
System.out where the rest of the test prints to System.err before throwing the exception.
Updated.
http://cr.openjdk.java.net/~sherman/8011647/webrev/
Thanks!
-Sherman
