Clojure

adds docstring support to defonce

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.5
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Vetted

Description

Pass all args from defonce on to def so it supports docstrings (or potentially other future features) just like def.

Docstrings and other Var metadata will be lost when the defonce is reëvaluated.

Patch: clj-1148-defonce-6.patch

Screened by:

Attachments

  1. Text File
    0001-new-defonce-hotness.patch
    17/Jan/13 5:16 PM
    1 kB
    Joe Gallo
  2. Text File
    clj-1148-defonce-2.patch
    29/Dec/13 10:24 PM
    3 kB
    Alex Miller
  3. Text File
    clj-1148-defonce-3.patch
    10/Jan/14 5:04 PM
    3 kB
    Alex Miller
  4. Text File
    clj-1148-defonce-4.patch
    28/Aug/14 12:08 PM
    3 kB
    Linus Ericsson
  5. Text File
    clj-1148-defonce-4.patch
    28/Aug/14 12:07 PM
    3 kB
    Linus Ericsson
  6. Text File
    clj-1148-defonce-5.patch
    28/Aug/14 12:30 PM
    3 kB
    Linus Ericsson
  7. Text File
    clj-1148-defonce-6.patch
    09/Sep/14 4:27 AM
    1 kB
    Linus Ericsson
  8. Text File
    defonce_fixes.patch
    24/Oct/13 12:44 PM
    3 kB
    Joe Gallo

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Alex Miller added a comment -

Changed to defect for stomping metadata.

Show
Alex Miller added a comment - Changed to defect for stomping metadata.
Hide
Permalink
Stuart Halloway added a comment -

Please add tests. The clojure.test-helper namespace has useful temporary namespace support.

Show
Stuart Halloway added a comment - Please add tests. The clojure.test-helper namespace has useful temporary namespace support.
Hide
Permalink
Joe Gallo added a comment -

This new patch includes the changes to defonce and also tests.

Show
Joe Gallo added a comment - This new patch includes the changes to defonce and also tests.
Hide
Permalink
Alex Miller added a comment -

Changing to Vetted so this is screenable again.

Show
Alex Miller added a comment - Changing to Vetted so this is screenable again.
Hide
Permalink
Rich Hickey added a comment -

I disagree about the stomp metadata - different metadata was provided. The purpose of defonce is to avoid the re-evaluation of the init. Is this the simplest change that accomplishes the doc string? In any case split in two.

Show
Rich Hickey added a comment - I disagree about the stomp metadata - different metadata was provided. The purpose of defonce is to avoid the re-evaluation of the init. Is this the simplest change that accomplishes the doc string? In any case split in two.
Hide
Permalink
Alex Miller added a comment -

Reduced scope of ticket to just passing defonce args on to def to add support for docstring. Added new patch that does this.

Show
Alex Miller added a comment - Reduced scope of ticket to just passing defonce args on to def to add support for docstring. Added new patch that does this.
Hide
Permalink
Stuart Sierra added a comment -

Screened clj-1148-defonce-2.patch but returning to 'incomplete' status.

The :arglists metadata in this patch (a list of symbols) is inconsistent with all other uses of :arglists (a list of vectors).

Other than that the patch is good.

Show
Stuart Sierra added a comment - Screened clj-1148-defonce-2.patch but returning to 'incomplete' status. The :arglists metadata in this patch (a list of symbols) is inconsistent with all other uses of :arglists (a list of vectors). Other than that the patch is good.
Hide
Permalink
Alex Miller added a comment -

Updated patch to address inconsistency in arglist format and attached clj-1148-defonce-3.patch.

Show
Alex Miller added a comment - Updated patch to address inconsistency in arglist format and attached clj-1148-defonce-3.patch.
Hide
Permalink
Stuart Sierra added a comment -

The patch clj-1148-defonce-3.patch is OK but it doesn't really address the docstring issue because defonce still destroys metadata. For example:

user=> (defonce foo "docstring for foo" (do (prn 42) 42))
42
#'user/foo
user=> (doc foo)
-------------------------
user/foo
  docstring for foo
nil
user=> (defonce foo "docstring for foo" (do (prn 42) 42))
nil
user=> (doc foo)
-------------------------
user/foo
  nil
Show
Stuart Sierra added a comment - The patch clj-1148-defonce-3.patch is OK but it doesn't really address the docstring issue because defonce still destroys metadata. For example: 
user=> (defonce foo "docstring for foo" (do (prn 42) 42))
42
#'user/foo
user=> (doc foo)
-------------------------
user/foo
  docstring for foo
nil
user=> (defonce foo "docstring for foo" (do (prn 42) 42))
nil
user=> (doc foo)
-------------------------
user/foo
  nil
Hide
Permalink
Stuart Sierra added a comment -

Screened with reservations noted.

Show
Stuart Sierra added a comment - Screened with reservations noted.
Hide
Permalink
Rich Hickey added a comment -

Stuart is right, second defonce should retain the doc string (since it again provides it, should be no-op)

Show
Rich Hickey added a comment - Stuart is right, second defonce should retain the doc string (since it again provides it, should be no-op)
Hide
Permalink
Alex Miller added a comment -

pull out of 1.6

Show
Alex Miller added a comment - pull out of 1.6
Hide
Permalink
Linus Ericsson added a comment - - edited

This version looks for previously defined var with resolve. A repeated defonce won't affect the namespace at all if the variable is already defined and bounded.

Please confirm using (resolve '~name) is not a problem w.r.t ns-bindings or similar.

This patch also contains the tests from clj-1148-defonce-3.patch as well as the :arglists property.

(patch 4 missed one def-row, sorry for mailbox noise).

Show
Linus Ericsson added a comment - - edited This version looks for previously defined var with resolve. A repeated defonce won't affect the namespace at all if the variable is already defined and bounded. Please confirm using (resolve '~name) is not a problem w.r.t ns-bindings or similar. This patch also contains the tests from clj-1148-defonce-3.patch as well as the :arglists property. (patch 4 missed one def-row, sorry for mailbox noise).
Hide
Permalink
Linus Ericsson added a comment -

Yet another, simpler version of defonce. No test-cases included.

This version just makes an (or (nil? v#) (not (.hasRoot v#)) test on the resolved variable. If this is true, really define by (def ~name ~@args) else do nothing.

Show
Linus Ericsson added a comment - Yet another, simpler version of defonce. No test-cases included. This version just makes an (or (nil? v#) (not (.hasRoot v#)) test on the resolved variable. If this is true, really define by (def ~name ~@args) else do nothing.
Hide
Permalink
Andy Fingerhut added a comment - - edited

Linus, while JIRA can handle multiple attachments on the same ticket with identical names, it can be confusing to do so. Would you mind renaming or removing the ones you have added with duplicate names, e.g. delete obsolete ones, perhaps? Instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Show
Andy Fingerhut added a comment - - edited Linus, while JIRA can handle multiple attachments on the same ticket with identical names, it can be confusing to do so. Would you mind renaming or removing the ones you have added with duplicate names, e.g. delete obsolete ones, perhaps? Instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches
Hide
Permalink
Sean Corfield added a comment -

On noticing that we have a JIRA issue at work to add docstrings to the handful of defonce calls we have (via alter-meta!), I came upon this issue and wondered what is stopping it moving forward?

Is Linus's patch an acceptable solution overall (and is just missing tests)? Or is a different approach wanted? Is this blocked by CLJ-1446 and folks want that fixed before updating defonce?

Show
Sean Corfield added a comment - On noticing that we have a JIRA issue at work to add docstrings to the handful of defonce calls we have (via alter-meta!), I came upon this issue and wondered what is stopping it moving forward? Is Linus's patch an acceptable solution overall (and is just missing tests)? Or is a different approach wanted? Is this blocked by CLJ-1446 and folks want that fixed before updating defonce?

People

Vote (14)
Watch (7)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.