Add some docstrings to clojure.cljs and eval.clj namespaces #93
Conversation
sbauer322
force-pushed
the
plugin-docstring
branch
from
6339075 to
2ae3018
Compare
|
On November 12, 2016, I will be merging this pull request If no one responds or comments before then. It would be nice to have feedback to confirm everything is in order. |
|
Last call @LightTable/maintainers before this is merged! |
|
Taking a look |
|
|
||
| (defn truncate [v] | ||
| (defn truncate | ||
| "Useless function... Same as 'identity'. Deprecated and to be removed." |
There was a problem hiding this comment.
Worth removing as this is needless commentary on a fn that is self explanatory. Also, dev comments/todos don't belong in fn docstrings
There was a problem hiding this comment.
Should we keep "Same as 'identity'. Deprecated and to be removed." at least? Letting users know the function is to be deprecated and that there already exists an alternative seems reasonable.
If the function docstring is not an appropriate place for this then where would be?
src/lt/plugins/clojure.cljs
Outdated
|
|
||
| (behavior ::cljs-result.inline-at-cursor | ||
| :desc "Clojurescript: Show the resulting evaluation inline at cursor location" | ||
| :doc "This is probably the same as `::cljs-result.inline` but puts the result widget somewhere else." |
There was a problem hiding this comment.
Please remove docs that aren't sure of functionality, here and other :doc keys that use "probably"
src/lt/plugins/clojure.cljs
Outdated
|
|
||
| (behavior ::cljs-watch-src | ||
| :desc "Clojurescript: Wraps the watched source code" | ||
| :doc "Wraps watched code with a call to `js/lttools.watch` to catch its result and |
There was a problem hiding this comment.
Documentation shouldn't be specifying implementation. Please replacejs/ltools.watch with a description of what it does or recommend removing :doc
src/lt/plugins/clojure.cljs
Outdated
|
|
||
| (behavior ::clj-watch-src | ||
| :desc "Clojure: Wraps the watched source code" | ||
| :doc "Wraps watched code with a call to `lighttable.nrepl.eval/watch` to catch |
There was a problem hiding this comment.
Same feedback as for :cljs-watch-src
src/lt/plugins/clojure.cljs
Outdated
| :print-length (object/raise-reduce editor :clojure.print-length+ nil) | ||
| :code (watches/watched-range editor nil nil nil))}))) | ||
| (behavior ::on-eval.cljs | ||
| :desc "Clojurescript: Eval editor content" |
There was a problem hiding this comment.
Please change all desc keys to use "Clojure:". Our :desc convention has been to use the prefix for plugin name, not as a language descriptor
There was a problem hiding this comment.
Will do - Could you point me to where we specify the :desc convention?
There was a problem hiding this comment.
Dunno if it's explicitly stated anywhere. Probably worth documenting in wiki/docs. I'd recommend reading more plugins and especially writing a plugin to familiarize yourself with conventions
There was a problem hiding this comment.
I have opened LightTable/LightTable#2296 to document this.
src/lt/plugins/clojure.cljs
Outdated
| (string/replace #"__\|(.*)\|__" watch))) | ||
|
|
||
| (behavior ::cljs-watch-custom-src | ||
| :desc "Clojurescript: Prepare exp for watching" |
There was a problem hiding this comment.
We should avoid using aliases like exp until we have a common place for users to look for commonly used aliases. exp -> expression should work
| (object/raise editor :editor.doc.show! result))))) | ||
|
|
||
| (behavior ::clj-doc-search | ||
| :doc "Links the 'Search language docs' input-text on the sidebar with a trigger to |
There was a problem hiding this comment.
Please convert to :desc. While I'm ok using :doc as an additive key we shouldn't be using it as a replacement for :desc. Same feedback for other behaviors that only have :doc
sbauer322
force-pushed
the
plugin-docstring
branch
2 times, most recently
from
226eb4f to
6dd1e0a
Compare
|
@cldwalker the requested changes have been addressed. |
src/lt/plugins/clojure.cljs
Outdated
| (fill-watch-placeholders (:exp opts) src meta watch)))) | ||
|
|
||
| (behavior ::clj-watch-custom-src | ||
| :desc "Clojure: Prepare exp for watching" |
There was a problem hiding this comment.
Worth updating exp to expression here as well
|
|
||
| (defn truncate [v] | ||
| (defn truncate | ||
| "Same as 'identity'. Deprecated and to be removed." |
There was a problem hiding this comment.
As mentioned before, please remove. The function is self explanatory and the person who added has no knowledge of when this is to be deprecated and removed. Code todos should not be accepted from external contributors who have little to no relevant contributions
sbauer322
force-pushed
the
plugin-docstring
branch
from
6dd1e0a to
aa87786
Compare
|
Ready for you, @cldwalker! |
|
Nice work! Let's ship it 👍 🚢 🌊 |
3 participants
This stems from @carocad's watch docstring PR, #89, as I found no clean way to append to his PR. The original will be closed.
The main difference is the usage of the
:dockey for behaviors when a lot of information not suited for the:desckey occurred. The:dockey will officially be used for additional optional documentation once this Light Table PR is merged: LightTable/LightTable#2276There is also various minor formatting changes.