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 a spec browser #2025

Merged
merged 1 commit into from
Jul 7, 2017
Merged

Add a spec browser #2025

merged 1 commit into from
Jul 7, 2017

Conversation

jpmonettas
Copy link
Contributor

Added a spec browser thru cider-browser-spec and cider-browse-spec-all.

The browser uses the functionality provided by the cider-nrepl spec middleware (see clojure-emacs/cider-nrepl#423) wich provides spec-list, spec-form and spec-example.

Using this it creates a buffer, a major mode, and a navigation stack to let you navigate into sub specs and back using keyboard or mouse, functionality that has been asked for in issues like:

#1918
#1919
#1901

Drawing the buffer uses cider-browse-spec--pprint that prints the spec form nicer by applying
rules to specific spec forms, hiding or shortening ns, etc. I don't like the current implementation, I think this can be improved, but I think it's important to do a custom print of different kinds of specs and specs combinators to have a better user experience.

Buttercup tests are comming!

It's the first time I create a pull request for Cider so probably I'm missing something.
Thanks for all the hard work on Cider btw!


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our [contribution guidelines][1]
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (make test)
  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)
  • You've updated the refcard (if you made changes to the commands listed there)

@bbatsov
Copy link
Member

bbatsov commented Jul 5, 2017

Can you add a few screenshot of the new functionality here as a reference?

@arichiardi
Copy link
Contributor

Awesome! This is going to be great ;)

@jpmonettas
Copy link
Contributor Author

all-specs
completing
multi
ring-headers
ring-request
ring-specs

@jpmonettas jpmonettas closed this Jul 5, 2017
@jpmonettas jpmonettas reopened this Jul 5, 2017
@jpmonettas
Copy link
Contributor Author

Sorry clicked the wrong button and closed the PR
@bbatsov are them ok?

@expez
Copy link
Member

expez commented Jul 6, 2017

Oh man, this is so cool, @jpmonettas! Fantastic work! 👍

(cider-browse-spec--propertize-fn spec-name)))))
(goto-char (point-min)))))

(defun ns-keywordp (str)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clojure calls this qualified-keyword? in 1.9 and we might want to stick with that, i.e. qualfied-keyword-p

;; and remove all clojure.core ns
(thread-last form
(replace-regexp-in-string "^\\(clojure.spec\\|clojure.spec.alpha\\)/" "s/")
(replace-regexp-in-string "^\\(clojure.core\\)/" "")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super helpful! 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I even wonder if this one should go in clojure-mode somehow or some place where it can be added to inf-clojure as well...need to think about that

(provide 'cider-browse-spec)


(provide 'cider-browse-spec)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double provide here

@expez
Copy link
Member

expez commented Jul 6, 2017

I was excited at the screenshots, but even more so after reading the code. Beautiful work, @jpmonettas!

This needs a rebase on master, but otherwise looks like it's ready for a merge and testing by early (eager?) adopters.

@bbatsov
Copy link
Member

bbatsov commented Jul 6, 2017

You'll have to rebase on top of the current master.

(push cider-browse-spec-buffer cider-ancillary-buffers)
(push cider-browse-spec-example-buffer cider-ancillary-buffers)

(defvar cider-browse-spec-navigation '()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be buffer-local?

Copy link
Contributor Author

@jpmonettas jpmonettas Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm if I make this buffer-local I'll need to surround code that works with the navigation state inside (with-current-buffer ) and for me when I see that I'm expecting that piece of code to read or write the buffer. Am I right?

;; Non interactive functions

(defun cider-browse-spec--clear-nav-history
()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is Elisp - params should be on the previous line. :-)


(defun spec-fn-p (value fn-name)
"Return non nil if VALUE is clojure.spec.[alpha]/FN-NAME."
(and (stringp fn-name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the the type check needed? Seems unlispy to me. Same for the previous function.


(defun cider-browse-spec--pprint (form)
"Given a spec FORM builds a multi line string with a pretty render of that FORM."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This this blank line.


(cond ((ns-keywordp form)
(cider-browse-spec--propertize-keyword form))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general kill all such blank lines. They make the code harder to read IMO.

@bbatsov
Copy link
Member

bbatsov commented Jul 6, 2017

A couple of other thoughts:

  • wouldn't an interface similar to that for apropos make more sense for this?
  • this has to be documented in the manual and added to mode menus
  • can you do anything with the spec view buffer? also - perhaps you should be able to jump from it from the doc buffer (right now we dump specs there if present)

jpmonettas added a commit to jpmonettas/cider that referenced this pull request Jul 6, 2017
@jpmonettas
Copy link
Contributor Author

jpmonettas commented Jul 6, 2017

I just pushed the code changes suggested above. Regarding @bbatsov thoughts:

wouldn't an interface similar to that for apropos make more sense for this?

didn't fully understood that, do you mean that cider-browse-spec-all should receive a query that can be a regex or a list of words and concat them in a regex?

this has to be documented in the manual and added to mode menus

under what menu should I put this commands?

perhaps you should be able to jump from it from the doc buffer (right now we dump specs there if present)

I can do something for that, but should't the specs in the doc come from the fns in the spec middleware (so everything comes from the same place) and be pprinted with the same rules? I'm thinking about things like multi-spec, you can't browse them otherwise

@bbatsov
Copy link
Member

bbatsov commented Jul 7, 2017

didn't fully understood that, do you mean that cider-browse-spec-all should receive a query that can be a regex or a list of words and concat them in a regex?

Yeah, that's one aspect of it. The other was that visually the buffer could be made to look more similar to apropos, but that's not a big deal. In general I really dislike this type of UI (the first browser was put together very quickly and we never got to improving it, but we did copy this for other browsers; and at some point I was planning to extract a bit of common code). Anyways, better some solution that works, than nothing.

under what menu should I put this commands?

Guess you can put those somewhere near the ns browser references.

@bbatsov
Copy link
Member

bbatsov commented Jul 7, 2017

I can do something for that, but should't the specs in the doc come from the fns in the spec middleware (so everything comes from the same place) and be pprinted with the same rules? I'm thinking about things like multi-spec, you can't browse them otherwise

Absolutely! That's certainly be an improvement over the current situation.

@jpmonettas
Copy link
Contributor Author

@bbatsov @expez I just pushed those changes, if you agree after it's merged I'll create an issue like "Make cider-doc use spec middleware functionality to retrieve and print spec part of the doc buffer"
and work on that together with #1924 cider-cloogle ??? :P

@jpmonettas jpmonettas force-pushed the feature/spec branch 3 times, most recently from cad1fbc to 11b543b Compare July 7, 2017 15:20
@bbatsov bbatsov merged commit 746038f into clojure-emacs:master Jul 7, 2017
@bbatsov
Copy link
Member

bbatsov commented Jul 7, 2017

A huge thanks for working on this from me! Hopefully we'll see more such awesome contributions from you!

I just pushed those changes, if you agree after it's merged I'll create an issue like "Make cider-doc use spec middleware functionality to retrieve and print spec part of the doc buffer"
and work on that together with #1924 cider-cloogle ??? :P

Sounds like a plan to me. Don't forget about the manual as well. ;-)

@jmayaalv
Copy link
Contributor

jmayaalv commented Jul 7, 2017

this is amazing! thanks a lot!

@arichiardi
Copy link
Contributor

First of all, thanks @jpmonettas, I am going to pull master for playing with this feature... it is awesome.

Yeah, that's one aspect of it. The other was that visually the buffer could be made to look more similar to apropos, but that's not a big deal. In general I really dislike this type of UI (the first browser was put together very quickly and we never got to improving it, but we did copy this for other browsers; and at some point I was planning to extract a bit of common code). Anyways, better some solution that works, than nothing.

This is an idea I had as well. what if we create an inf-clojure-extras that enables these kind of things in there too, but with a pure console-like UX. An apropos on steroids.

@jpmonettas
Copy link
Contributor Author

jpmonettas commented Jul 7, 2017

@bbatsov @expez @arichiardi hey glad I can help. First I should say thanks to you guys for all the hard work on the tool I've been using the past 5 years everyday.

I've some free time now so I'll be trying to contribute to the proyect wherever I can.
Saludos desde Uruguay!

xiongtx pushed a commit to clojure-emacs/helm-cider that referenced this pull request Jul 8, 2017
xiongtx pushed a commit to clojure-emacs/helm-cider that referenced this pull request Jul 8, 2017
xiongtx pushed a commit to clojure-emacs/helm-cider that referenced this pull request Jul 8, 2017
xiongtx pushed a commit to clojure-emacs/helm-cider that referenced this pull request Jul 8, 2017
xiongtx added a commit to clojure-emacs/helm-cider that referenced this pull request Jul 8, 2017
Spec browsing is introduced in CIDER by clojure-emacs/cider#2025.
@xiongtx xiongtx mentioned this pull request Mar 14, 2018
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.

5 participants