Four functions in clojure.instant have incomplete documentation

Description

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:

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

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
Screened: Alex Miller

Environment

None

Activity

Show:
Bruce Adams
October 15, 2016, 6:40 PM

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

Bruce Adams
October 16, 2016, 11: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.

Alex Miller
October 17, 2016, 3:53 PM

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.

Bruce Adams
October 23, 2016, 10:06 PM

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

Completed

Assignee

Bruce Adams

Reporter

Bruce Adams

Labels

Approval

Ok

Patch

Code

Fix versions

Priority

Major
Configure