Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add docstrings to vars in re-frame.core #257

Closed
wants to merge 1 commit into from

Conversation

grammati
Copy link

Cider is able to show docstrings of clojurescript functions (using C-d C-d d), but for most functions in re-frame.core, it just shows "Not documented", which is sad and kind of ironic given that re-frame is probably the best-documented project in all of clojuredom.

This commit adds a tiny macro (that I called defvar only because I couldn't think of anything better) which copies over the :doc metadata from the implementation function.

@danielcompton
Copy link
Contributor

This looks neat! With Cider, what do you need running for this to work? Does it need a CLJS REPL or can it use a CLJ one because re-frame uses CLJC?

@grammati
Copy link
Author

I tried in both a clj and a cljs repl and was able to get the docs in both.

@danielcompton
Copy link
Contributor

Sorry for the delay on this. We're still considering this internally as it is a bit of a change adding a custom macro here.

@danielcompton
Copy link
Contributor

danielcompton commented Dec 5, 2016

Hey, I've delved into this a bit deeper. I had a go at this in a CLJS figwheel REPL, and I couldn't get docs for the defvar'd vars. How did you get the docs in a CLJS repl?

If we're going to make this change, it would also be really handy to get the other metadata from the functions, particularly the arglist so that we can generate docs with codox. I've had a look at getting the other metadata but ran out of time. I suspect that copying how cljs.repl/doc works would be the way to do it.

If you want to get docs from Clojure REPL's as well, then you may want to make the macro one that can run from Clojure and ClojureScript via https://groups.google.com/forum/#!topic/clojurescript/HsWTuhMP7yc.

I understand if you don't have the time/inclination to go this deep into it. Sorry for the delay.

@superstructor
Copy link
Contributor

We investigated this PR as a solution for #216 and #456. In the end, we had to reject this approach and resolve the issue another way because tooling like IntelliJ/Cursive or Jocker do static analysis of code and a macro expansion approach won’t work for them.

But, many thanks for the PR all the same @grammati! We looked at it carefully and your suggestion certainly helped us to research for this fix.

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants