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 evaluation in context like CIDER #434

Closed
liquidz opened this issue Aug 22, 2022 · 11 comments
Closed

Add support for evaluation in context like CIDER #434

liquidz opened this issue Aug 22, 2022 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@liquidz
Copy link
Owner

liquidz commented Aug 22, 2022

cf. clojure-emacs/cider#2113
cc @sheluchin

@liquidz liquidz added the enhancement New feature or request label Aug 22, 2022
liquidz added a commit that referenced this issue Aug 22, 2022
@liquidz
Copy link
Owner Author

liquidz commented Aug 22, 2022

@sheluchin I tried to add <Plug>(iced_eval_in_context) in feature/eval-in-context branch.

For example, you can define mappings as follows.

nmap <Leader>ece <Plug>(iced_eval_in_context)<Plug>(sexp_outer_list)

Could you try?

@sheluchin
Copy link

@liquidz Thank you for trying this.

It looks like it doesn't quite work in this version. In the simplest form:

(identity x)

produces this error:

(clojure.core/let [42] (identity x))
=> CompilerException
Syntax error macroexpanding clojure.core/let at (iced_test.clj:28:1).
[42] - failed: even-number-of-forms? at: [:bindings] spec: :clojure.core.specs.alpha/bindings

It looks like the iced#operation#eval_in_context function gets the entire evaluation context but doesn't do anything to figure out the individual bindings, so it misses the x part.

It should also be able to handle multiple bindings somehow.

@liquidz
Copy link
Owner Author

liquidz commented Aug 23, 2022

@sheluchin Thanks for your confirmation!
The current behavior is same as CIDER as far as I've actually tried in Emacs.

You should input x 42 (let-style) as context like this post in clojurian slack.
https://clojurians.slack.com/archives/C053AK3F9/p1661180273024399?thread_ts=1661168165.432639&cid=C053AK3F9
image
image (1)

It looks like the iced#operation#eval_in_context function gets the entire evaluation context but doesn't do anything to figure out the individual bindings, so it misses the x part.

I think the approach to figure out bindings automatically is not appropriate because of the order of bindings.

For example, if we pass 1 2 3 as context for the form (+ a b c), we would expect a = 1, b = 2, c = 3.
But, it is tired to specify the appropriate values in the appropriate order when the form is large or has many bindings.

In comparison, it would be better to simply input a 1 b 2 c 3 like clojure.core/let, so that we don't have to be aware of the binding order.
And even if the form is changed as (* x (+ a b c)), we can evaluate it by simply adding x 4, as in a 1 b 2 c 3 x 4.

It should also be able to handle multiple bindings somehow.

As described above, we can handle multiple bindings like a 1 b 2 c 3 with let-style.

@sheluchin
Copy link

Ah, my mistake! Yes, as you describe - providing the full binding form - it does work as expected and is a nice workflow tool. I like the idea of not having to make edits to the file when testing stuff quickly in the REPL.

It's nice that you can scroll through the history with up and down arrows too :) Do you think it would be difficult to add an FZF interface for the history?

Thanks again, @liquidz. It's good stuff.

@liquidz
Copy link
Owner Author

liquidz commented Aug 24, 2022

Do you think it would be difficult to add an FZF interface for the history?

Cool.
:history i command shows the history of input, so I think it can be supported.
But it may be needed to define another mapping.
I'll try.

@sheluchin
Copy link

I think I found a small bug in the current version. It looks like if you start entering bindings and then press escape, it still tries to evaluate it, rather than canceling.

liquidz added a commit that referenced this issue Aug 24, 2022
@liquidz
Copy link
Owner Author

liquidz commented Aug 24, 2022

@sheluchin Nice catch! Thanks.
I've fixed in in feature/eval-in-context branch.

@liquidz
Copy link
Owner Author

liquidz commented Aug 24, 2022

@sheluchin

Do you think it would be difficult to add an FZF interface for the history?

I've added <Plug>(iced_eval_in_selected_context) in feature/eval-in-context branch for trial. (d3c5f03)

E.g.

nmap <Leader>esce <Plug>(iced_eval_in_selected_context)<Plug>(sexp_outer_list)

With this mapping, we can evaluate codes with the context selected by selector.
cf. https://liquidz.github.io/vim-iced/vim-iced.html#g%3Aiced%23selector%23search_order

But we cannot switch between <Plug>(iced_eval_in_context) and <Plug>(iced_eval_in_selected_context).
So it is difficult to realize the case where a user has filtered by selector, but still wants to enter a different context.
It might be possible to create an original input buffer, but I don't think it would be worth the effort.

Thus, the approach providing different mappings does not seems to be a good, as it is tired to use different mappings for inputting and selecting.
It may be smarter to use <C-e>, vim's buit-in function, if we want to select from the history while entering context.

How do you think?

@sheluchin
Copy link

I've fixed in in feature/eval-in-context branch.

Thanks! Works.

But we cannot switch between (iced_eval_in_context) and (iced_eval_in_selected_context).

I see what you mean. It is awkward to make a selection and then modify it in some way. I don't think it's very important as <Plug>(iced_eval_in_context) + <C-f> already makes search + edit fairly easy. Adding fuzzy search on top would be a nice QOL improvement, but it's not a big deal.

However, just so you're aware, FZF does have an option for dealing with this kind of situation. You can see it with the :History: and :History/ commands from fzf.vim. It looks like this:

image

Detail.

#iced#selector has to be able to handle the other selectors as well, so I see how that could make it more complicated.

In any case, this is a nice new feature :) Thanks again, @liquidz.

@liquidz
Copy link
Owner Author

liquidz commented Aug 25, 2022

@sheluchin Thanks for your confirmation!

However, just so you're aware, FZF does have an option for dealing with this kind of situation. You can see it with the :History: and :History/ commands from fzf.vim. It looks like this:

Ah I see, it is editable after selection.

I don't think it's very important as (iced_eval_in_context) + already makes search + edit fairly easy. Adding fuzzy search on top would be a nice QOL improvement, but it's not a big deal.

OK. I'll delete <Plug>(iced_eval_in_selected_context) for now, and will merge to dev branch.
If we find a good way to achieve a method like fzf.vim for supported selectors, I'll consider adding it again.

@liquidz liquidz self-assigned this Aug 25, 2022
@liquidz
Copy link
Owner Author

liquidz commented Aug 29, 2022

Just released v3.11.3086

@liquidz liquidz closed this as completed Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants