Fork me on GitHub
#cljs-dev
<
2018-10-03
>
Ethan Miller13:10:22

Hi All, I'd like to help out with clojurescript! I picked a newbie docstring issue to start: https://dev.clojure.org/jira/browse/CLJS-2209. Please let me know if this isn't the appropriate place to ask for initial feedback. I'll attach the patch below, but here is what the patch yields:

(test-constant1 ... test-constantN)  result-expr

  If the expression is equal to a test-constant, the corresponding
  result-expr is returned. A single default expression can follow the
  clauses, and its value will be returned if no clause matches. If no
  default expression is provided and no clause matches, an Error is thrown.

  The test-constants are not evaluated. They must be compile-time
  literals, and need not be quoted. The only exception to this holds for
  the use of test-constants that are constant Vars (vars with metadata ^:const).
  In this case, the constant's value is inlined.

  Unlike cond and condp, case does a constant-time dispatch, the
  clauses are not considered sequentially.  All manner of constant...
Some questions/thoughts where feedback might be needed: * I moved the second paragraph of the docstring up because when I added the bit about constant Vars it pushed that section down too much and it seemed to me that that paragraph needs to come at the beginning anyway since it explains the basic logic of case. * Is it correct to describe the behavior around consant Vars as an "exception"? * Is it accurate to use the "constant Vars" instead of the "symbols that resolve to ^:const Vars". I made the substitution in the interest of using plain language. * Do we want to mention that the behavior differs from clj?

🍻 4
mfikes14:10:43

@ezmiller77 I haven’t reviewed the above in detail, but one sentiment: The facets where ClojureScript differs from Clojure were not, in hindsight, intentional. But nevertheless they should be documented. This makes me feel that they should be treated as “exceptional,” and perhaps given secondary prominence in the docstring. (Put last where possible, or otherwise lightly mixed in if awkward to do otherwise.)

mfikes15:10:12

@ezmiller77 Looking at your suggested copy, it LGTM. I think it is OK to refer to that facet as an "exception." I think your wording around constant Vars is OK. There is a small inconsistency in using Var once and then var. I would perhaps say "with ^:const metadata". It seems like you could say "test-constants that are symbols resolving to constant Vars." but perhaps that is needlessly verbose. Mentioning that it differs from Clojure for this aspect might be nice. Without this it seems reasonable to expect some users would expect this docstring may also be describing valid Clojure behavior.

đź‘Ť 4
Ethan Miller15:10:48

Thanks @U04VDQDDY! I'll see about some of the adjustments you suggested.

Ethan Miller21:10:39

@U04VDQDDY: I've updated the patch with your suggestions. Is the next step here to attach the patch to the Jira ticket?

mfikes21:10:52

Yeah, that sounds good.

mfikes15:10:44

Cool. It looks like the graaljs REPL in master still works with the recent Graal RC7 release. Additionally, the Python polyglot calls now work the first time without throwing an exception. (Previously you'd have to make a Python polyglot call, have it fail, and then try again and it would succeed.)