<< Back to previous view

[CLJ-2224] Support printing and reading of Java 8 java.time.Instant Created: 19/Aug/17  Updated: 20/Aug/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: David B├╝rgin Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: instant

Java 8

Approval: Triaged


In Clojure 1.9 alpha, limited support for Java 8 java.time.Instant was added, namely by (conditionally) extending the Inst protocol to that type.

It would be useful to enhance support for java.time.Instant further by

  • installing a print-method and print-dup for java.time.Instant
  • providing a read-instant function for reading java.time.Instant

This functionality is already provided in Clojure 1.8 today for the types java.util.Date, java.util.Calendar, and java.sql.Timestamp; extending it to java.time.Instant would be very helpful in environments using Java 8.

[CLJ-2044] Four functions in clojure.instant have incomplete documentation Created: 15/Oct/16  Updated: 24/Oct/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5, Release 1.6, Release 1.7, Release 1.8, Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Bruce Adams Assignee: Bruce Adams
Resolution: Unresolved Votes: 0
Labels: docstring, instant

Attachments: Text File defns-for-instant-def-timestamp.patch     Text File defns-for-instant.patch    
Patch: Code
Approval: Prescreened


Of the five public functions defined in clojure.instant, these four:

  • parse-timestamp
  • read-instant-calendar
  • read-instant-date
  • read-instant-timestamp

are each declared as a Var without any arglists metadata. This means documentation does not contain function calling information.

In http://clojure.github.io/clojure/clojure.instant-api.html, each of these functions is described as a var and there is no Usage: ... information given.

The output of doc does not include argument list information. For example:

user=> (doc clojure.instant/read-instant-date)
  To read an instant as a java.util.Date, bind *data-readers* to a map with
this var as the value for the 'inst key. The timezone offset will be used
to convert into UTC.

A related problem is that stack traces show anonymous functions instead of the names for any of these functions. For example:

user=> (clojure.instant/read-instant-timestamp "123")
RuntimeException Unrecognized date/time syntax: 123  clojure.instant/fn--6879/fn--6880 (instant.clj:107)

Proposed: Refactor the code into defn functions which makes the code clearer and addresses the documentation issue. An alternate approach would be to apply :arglists metadata to the vars.

Patch: defns-for-instant-def-timestamp.patch
Prescreened: Alex Miller

Comment by Bruce Adams [ 15/Oct/16 12:40 PM ]

Proposed solution: refactor the definitions of the four problematic functions to be defined using defn.

Comment by Bruce Adams [ 16/Oct/16 5:24 PM ]

Some of my thinking leading to the solution I propose.

Initially, when I realized that I didn't know what arguments parse-timestamp required, I assumed the appropriate fix was to enhance the docstring. Then I noticed that the on-line documentation for functions was formatted quite differently from the text output by doc. Any decent fix was going to have to feed function information into different variants of documentation formatting code.

I can guess, from other examples such as first, that :arglists metadata is what indicates that a var is a function. One possible solution would be to add :arglists to each of the four functions. It felt cleaner to refactor the code into simple defn functions. Refactoring code just for the side effect of documentation seems a bit odd, but the code itself strikes me as more legible after my refactoring.

Comment by Alex Miller [ 17/Oct/16 9:53 AM ]

This seems reasonable to me. I would move the timestamp regex into a separate (private) var instead of creating it in parse-timestamp.

It's possible the way these functions were defined was designed to minimize startup time or something like that, but I don't have any background on that.

Comment by Bruce Adams [ 23/Oct/16 4:06 PM ]

Updated patch based on Alex's great suggestion. This adds a separate, private, def for the timestamp regex.

[CLJ-926] Instant literals do not round trip correctly Created: 05/Feb/12  Updated: 17/Feb/12  Resolved: 17/Feb/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Cosmin Stejerean Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: instant, reader, tagged-literals

Attachments: Text File CLJ-926-round-trip-date-instants-with-tests.patch    
Patch: Code and Test
Approval: Screened
Waiting On: Stuart Sierra


When using java.util.Date for instant literals (which is the default) instants do not round-trip properly during daylight savings. Here is an example:

(read-string "#inst \"2010-11-12T13:14:15.666-06:00\"")
#inst "2010-11-13T06:14:15.666+10:00"

I'm currently in Melbourne, which is normally GMT+10. However, on November 12th daylight savings is in effect, so the proper GMT offset is +11. The date above is actually using the correct local time (6:14:15) but with the wrong offset. The problem is more obvious when you attempt to round-trip the instant that was read.

user> #inst "2010-11-13T06:14:15.666+10:00"
#inst "2010-11-13T07:14:15.666+10:00"

Notice that we read 6:14am but the output was 7:14 with the same offset. Upon digging deeper the real culprit seems to be the use of String.format (through clojure.core/format) when outputting java.util.Date. Notice the following inside caldate->rfc3339

(format "#inst \"%1$tFT%1$tT.%1$tL%1$tz\"" d))

Let's compare the timezone offset in the underlying date with what is printed by %1$tz

user> (def d #inst "2010-11-13T06:14:15.666+10:00")
user> (.getHours d)
user> (.getTimezoneOffset d)

For reference, the definition of getTimezoneOffset is

-(Calendar.get(Calendar.ZONE_OFFSET) + Calendar.get(Calendar.DST_OFFSET)) / (60 * 1000)

So far it looks good. 6am in GMT+10 becomes 7am in GMT+11. Let's see how String.format handles it though.

clojure.instant> (format "%1$tz" d)
clojure.instant> (format "%1$tT" d)

String.format prints the correct hour, but with the wrong offset. The recommended way for formatting dates is to use a DateFormatter.

The String.format approach appears to work properly for Calendar, but not for Date. Therefore the attached patch keeps the current
implementation for java.util.Calendar but uses SimpleDateFormat to handle java.util.Date correctly. This fixes the roundtrip problem.

Comment by Cosmin Stejerean [ 05/Feb/12 9:29 PM ]

Not quite sure how to create a repeatable test for this since the issue depends on the local timezone.

Comment by Steve Miner [ 06/Feb/12 8:33 AM ]

java.util.TimeZone/setDefault could be used for testing in various timezones.

Comment by Steve Miner [ 06/Feb/12 8:37 AM ]

Regarding the patch: SimpleDateFormat is a relatively heavy-weight object, so it seems bad to allocate one every time you print a Date. Unfortunately, SimpleDateFormat is not thread-safe, so you can't just share one. The Java world seems to agree that you should use JodaTime instead, but if you're stuck with the JDK, you need to have a ThreadLocal SimpleDateFormat. I'm working on my own patch along those lines.

Comment by Fogus [ 06/Feb/12 7:38 PM ]

Excellent analysis. I'll keep track of this case and vet any patches that come along. Please do attach a regression test if possible.

Comment by Cosmin Stejerean [ 06/Feb/12 8:39 PM ]

I attached a new patch using a SimpleDateFormat per thread using a thread local. I'll try to add some tests next.

Comment by Cosmin Stejerean [ 06/Feb/12 10:42 PM ]

A combined patch that uses a thread local SimpleDateFormat, tests round-tripping at a known daylight savings point (by overriding the default timezone) and checks round tripping at multiple points throughout the year (every hour of the first 4 weeks of every month of the year).

Steve, thanks for the suggestions on using a thread local and TimeZone/setDefault.

Comment by Steve Miner [ 07/Feb/12 1:32 PM ]

I filed CLJ-928 with my patch for fixing the printing of Date and Timestamp using UTC. I copied the tests from the CLJ-926 patch to make sure we're compatible.

Comment by Fogus [ 07/Feb/12 3:11 PM ]

Thanks for the regression test also. I'll vett this patch ASAP.

Comment by Fogus [ 17/Feb/12 1:24 PM ]

Seems reasonable to me.

Comment by Stuart Sierra [ 17/Feb/12 1:46 PM ]

Vetted. Will apply later.

Comment by Stuart Sierra [ 17/Feb/12 1:56 PM ]

Not Vetted. Test. That thing.

Comment by Stuart Sierra [ 17/Feb/12 2:10 PM ]

No, it's "Screened," not "Test." Somebody save me.

Comment by Stuart Sierra [ 17/Feb/12 3:55 PM ]

Superseded by CLJ-928.

Generated at Fri Sep 22 08:22:31 CDT 2017 using JIRA 4.4#649-r158309.