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 support for spec middleware (form, example) #335

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@christoph-frick
Copy link
Contributor

commented May 21, 2019

No description provided.

@christoph-frick

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

This is a first attempt in supporting the features from the spec middleware.

One problem so far is the cursor jumping to the beginning of the file after cs[fe].

So far i tested with:

(ns vim-fireplace-spec.core
  (:require [clojure.spec.alpha :as s]))

(alias 'self *ns*)

(s/def ::id int?)
(s/def ::name string?)
(s/def ::user (s/keys :req [::id ::name]))

#_(
   ::id
   ::self/id
   :vim-fireplace-spec.core/id
   ::name
   ::self/name
   :vim-fireplace-spec.core/name
   ::user
   ::self/user
   :vim-fireplace-spec.core/user
   )

#_ (->> (s/registry) (keys) (map str))
@tpope

This comment has been minimized.

Copy link
Owner

commented May 21, 2019

I'm out of the loop, can you link to the documentation of what this spec middleware is exactly?

The cursor jump is because functions in VimL return 0 by default, and :exe 0 is the same as :0 which jumps to the first line. return '' will fix it.

@christoph-frick

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Relevant links:

I have no clue how that looks in emacs.

Thanks for the hint about the jump. I'll fix that.

@christoph-frick christoph-frick force-pushed the christoph-frick:spec branch from 7b2dd0c to c37b5ab May 22, 2019

@christoph-frick

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

I have force-pushed the fix with the return "".

Show resolved Hide resolved plugin/fireplace.vim Outdated
Show resolved Hide resolved plugin/fireplace.vim Outdated

function! s:set_up_fireplace_spec() abort
call s:map('n', 'csf', '<Plug>FireplaceSpecForm')
call s:map('n', 'cse', '<Plug>FireplaceSpecExample')

This comment has been minimized.

Copy link
@tpope

tpope May 23, 2019

Owner

These maps conflict with surround.vim and thus are disqualified in my book. For spec-form, it might make sense to overload K.

I think more fundamental than maps is providing commands like :SpecName.

This comment has been minimized.

Copy link
@christoph-frick

christoph-frick May 27, 2019

Author Contributor

With K you mean adding a third branch into s:K and dispatch to SpecForm if word starts with :?

For now i'll remove the keyboard shortcuts and add the commands.

This comment has been minimized.

Copy link
@tpope

tpope May 27, 2019

Owner

Right. Currently pressing K on a keyword is worthless so why not?

This comment has been minimized.

Copy link
@christoph-frick

christoph-frick Jun 4, 2019

Author Contributor

Right. Currently pressing K on a keyword is worthless so why not?

For the records: this is actually the behaviour of clojure.repl/doc (if arg is a keyword, show spec/describe

return a:1
endfunction

function! fireplace#fully_qualified_symbol(symbol) abort

This comment has been minimized.

Copy link
@tpope

tpope May 27, 2019

Owner

qualify_keyword seems like a more accurate name.

endfunction

function! s:set_up_fireplace_spec() abort
command! -buffer -bar -nargs=? -complete=customlist,fireplace#eval_complete SpecForm :exe s:SpecForm(<q-args>)

This comment has been minimized.

Copy link
@tpope

tpope May 27, 2019

Owner

Let's leave this as -nargs=1 for now. I won't rule out the <cword> default but we should do it everywhere it makes sense if so.

christoph-frick added some commits May 28, 2019

@christoph-frick

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

I have addressed all your points

@@ -1669,41 +1681,33 @@ augroup END
" Section: Spec
" TODO: `spec-list`

This comment has been minimized.

Copy link
@tpope

tpope May 28, 2019

Owner

What's the story here? I would imagine that switching from maps to commands would make this one easy to implement?

This comment has been minimized.

Copy link
@christoph-frick

christoph-frick May 28, 2019

Author Contributor

Removed that comment. It's basically (spec/registry) and I assume, that in emacs/cider one opens that list and then navgiates to form/example from there. AFAIK there currently is nothing similar in fireplace to add this to.

@@ -57,11 +57,19 @@ DOCUMENTATION *fireplace-documentation*
:Javadoc {class} Open the java docs for the given class in a browser.

*fireplace-K*
K Look up docs for symbol under cursor.
K Look up (java) doc/spec-form for symbol/keyword under
cursor.

This comment has been minimized.

Copy link
@tpope

tpope May 28, 2019

Owner

I found this a tad hard to parse. How about Look up the doc, javadoc, or spec form for the identifier under the cursor.?

This comment has been minimized.

Copy link
@christoph-frick

christoph-frick May 28, 2019

Author Contributor

Sure

*fireplace-:SpecExample*
:SpecExample [kw] Generate example for spec at the cursor or given
argument

This comment has been minimized.

Copy link
@tpope

tpope May 28, 2019

Owner

:SpecForm {keyword}, and the wording needs tweaking but I'm not sure to what. Do these roughly correspond to clojure.spec functions? If so, it might make sense to reference that like we do for :FindDoc. I worry that just saying "spec" is confusing to those not familiar (it was for me).

This comment has been minimized.

Copy link
@christoph-frick

christoph-frick May 28, 2019

Author Contributor

We could use some codified version like (spec/form) - they are at least all named the same for now in all three versions of spec, that are in the wild.

This comment has been minimized.

Copy link
@christoph-frick

christoph-frick May 28, 2019

Author Contributor

I used spec/form and gen/generate - the later is the alias used in the main docs https://clojure.org/guides/spec#_sampling_generators; the first is a bit more clear than just s

This comment has been minimized.

Copy link
@tpope

tpope May 31, 2019

Owner

Can we go all in and use the full namespaces? That should make it easier to look up.

This comment has been minimized.

Copy link
@christoph-frick

christoph-frick Jun 1, 2019

Author Contributor

The problem with the namespace would be to name all two currently (and soon three) supported by orchard/cider (spec "0", spec alpha, spec alpha 2) - which is a problem on it's on (see answer to "error handling" comment below)

@tpope

This comment has been minimized.

Copy link
Owner

commented May 31, 2019

I'm afraid I saved the hardest part for last: error handling. We need to handle the case the op isn't there. This is the first time we don't have a fallback for if CIDER isn't installed, which is fine, but I need to think a bit on how to proceed.

We also need error handling for if the op is there but fails. What are the failure cases, and how can we handle them?

@christoph-frick

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

I'll list all i thought about so far:

  • Op is not there -> handled already, but we could handle this ourself (see below); use fallback (see below) or notify user?
  • a spec kw not known (currently we fail silently without giving the user feedback)
  • a spec kw not loaded yet (same as "not known", but might be confusing for the user) - I think we should keep it that way, since we can not know where the def is and load it for the user; there is autocompletion in the prompt which makes it more obvious
  • there is an error somewhere down the road and something throws -> think this is handled already: random errors bubble up
  • generating an example ends in an "infinite loop" - if the spec is complex or recursive and the user does not take care; the example gets huge, eats all the CPU and RAM. I'd expect CTRL-C works here and propagates well into cider (?)

As for the fallback: no cider or old cider: we can do the same as orchard does. We could Eval some "try what spec is there" with the given keyword.

The only problem I see with this (and it's one with orchard too): the search for the "right" spec lib just favours the order of existence (try alpha1, then spec0 - soon to be started with alpha2). Right now this is not a huge problem, because most likely folks will use alpha1. But once alpha2 hits, the code bases are spec1 and spec2 "is there" and maybe incompatible and in the worst case scenario messes things up if loaded in parallel. So that could mean, a switch per project is needed to tell the tooling, which spec version is in use or have the tooling do more work in finding the right one.

@tpope

This comment has been minimized.

Copy link
Owner

commented Jun 3, 2019

The current "op is not there" behavior is silent failure too. We need separate errors for both not connected and connected but no op. This is the thing I will think on. (If you want to provide an eval callback, great, but I'm inclined to think it's not worth bothering with.)

The only other case I care about is "not known". The others seem fine as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.