Skip to content

Conversation

@vspinu
Copy link
Contributor

@vspinu vspinu commented Sep 19, 2014

Before continuing on #815 I couldn't stop myself from cleaning up the interactive eval API.

There is now one API entry point - cider-interactive-eval. All other commands are based on it. I am removing a couple of unnecessary API functions. Entries like cider-interactive-eval-in-repl add naming redundancy with the corresponding handler. You can simply call cider-interactive-eval with eval-in-repl handler and the code will be just as clear.


Other changes:

  • The recent "dummy-file" change to interactive eval touched only two interactive
    eval functions. This patch completes the change.
  • Simplify the code by removing unused or unnecessary API functions:
    cider-interactive-eval-print, cider-popup-eval-print-handler,
    cider-popup-eval-print, cider-interactive-eval-to-repl
  • Rename cider-interactive-eval-print-handler -> cider-eval-print-handler.
  • Bring all interactive eval functionality into one place and arrange
    functions in a logical order.
  • buffer argument to interactive eval handlers is optional

cider-client.el Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This op those not support the ns param. See the reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This op those not support the ns param. See the reference.

It does not but it return "ns" in the response and if it's not supplied
the response ns is either "user" or some other arbitrary (as far as I
can tell) ns.

Cider relies on returned ns for setting nrepl-buffer-ns. So this
actually fixes #814. Forgot to mention that.

I don't like nrepl-buffer-ns tracking as it's currently implemented
and if I understand correctly you are not found of it either. But for
time being let this be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, all that is true, but I have just discovered that we cannot keep the "ns" as an argument to :load-file op. It doesn't work on first evaluation status ("namespace-not-found" "error" "done"). So the nrepl-buffer-ns will stay "user" unless we remove its auto assignment in nrepl-dbind-response. In any case this will not cause any issues with interactive evaluation because nrepl-buffer-ns is no longer used for that.

@bbatsov
Copy link
Member

bbatsov commented Sep 19, 2014

Apart from my small remarks, the code looks good.

@vspinu
Copy link
Contributor Author

vspinu commented Sep 20, 2014

I have addressed all your comments. Thanks.

  - There is now one API entry point - `cider-interactive-eval`. All other
    commands are based on it.

  - Simplify the code by removing unused or unnecessary API functions:
    `cider-interactive-eval-print`, `cider-popup-eval-print-handler`,
    `cider-popup-eval-print`, `cider-interactive-eval-to-repl`

  - Rename `cider-interactive-eval-print-handler` -> `cider-eval-print-handler`.

  - Bring all interactive eval functionality into one place and arrange
    functions in a logical order.

  - `buffer` argument to interactive eval handlers is optional
bbatsov added a commit that referenced this pull request Sep 20, 2014
Refactor the interactive evaluation API
@bbatsov bbatsov merged commit f7569f5 into clojure-emacs:master Sep 20, 2014
@vspinu vspinu deleted the ieval branch September 20, 2014 22:12
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.

2 participants