emestee / SublimeREPL

SublimeREPL - run an interpreter inside ST2 (Clojure, CoffeeScript, F#, Groovy, Haskell, Lua, MozRepl, NodeJS, Python, R, Ruby, Scala, shell or configure one yourself)
https://github.com/wuub/SublimeREPL
Other
12 stars 0 forks source link

inconsistent behaviour when evaluating (ns ...) form #10

Open roti opened 11 years ago

roti commented 11 years ago

Evaluating (ns ...) from a buffer (i.e. using ctrl+,,s) and directly in the REPL yields different results. Take following example:

  1. Current namespace is user, REPL prompt is user >
  2. Evaluate from buffer (i.e. using ctrl+,,s)
(ns myns.core 
    (require [clojure.string :as string]))

nil is printed in the REPL, but the prompt remains user >. I worked with emacs before, and I remember that at this point the current namespace should be myns.core (i.e. the prompt should be myns.core >)

  1. Evaluate *ns* from REPL #<Namespace user> is printed, which proves that current namespace remains user, as indicated by the prompt.
  2. Evaluate from the same buffer
(def myjoin string/join)

#'myns.core/myjoin is printed in the REPL, even though prompt was (and remains) user >. This is ok. It proves that the previous form was evaluated correctly and that evaluation is done in the correct namespace. I suppose that SublimeREPL knows in which namespace to evaluate by looking at the buffer.

If at step 2, I evaluate the form directly in the REPL, and not from the buffer, the prompt changes to myns.core >, indicating that the current namespace has changed from user to myns.core.

I see a problem with the inconsistent behavior at step 2, i.e. it should not matter if I evaluate the form from a buffer or write it directly in the REPL. I don't expect the behavior to be like in emacs (although it would be nice).

emestee commented 11 years ago

Thank you, I am looking into this.

emestee commented 11 years ago

The reason this is happening is because of a text transfer wrapper (C-,,s etc) inherited from previous implementation. A trivial fix for your particular problem is to add:

text = text.strip()
if text.startswith('(ns '):
    return default_sender(repl, text + repl.cmd_postfix, view)

immediately in the beginning of def clojure_sender in text_transfer.py

However a proper resolution has larger implications because I suspect people have different ideas about what should happen when in-editor statements are evaluated. The current implementation actually overrides the current namespace, and the namespace of the outer statement, in favour of the namespace of the file, as it assumes the lein namespace per file convention. If no such convention is observed, then the code barfs. This makes it apparently impossible to evaluate code from e.g. scratch buffers. As I am not very familiar with Clojure, I can not conceive all possible use cases and implications of altering the current implementation. I am eager for any advice which will help me understand those.

emestee commented 11 years ago

What actually happens with your code if it is evaluated from a file with (ns somenamespace) on top:

 (binding [*ns* (or (find-ns 'somenamespace) (find-ns 'user))] (load-string "(ns myns.core 

(require [clojure.string :as string]))"))

Without the namespace:

(load-string "(ns myns.core (require [clojure.string :as string]))")

As you can see, the text transfer handler wraps the statements in (load-string), which does not propagate the current namespace back from loaded code. The stated reason for (load-string) is to send the entire piece of code as a single syntactic unit, and detect syntax errors before evaluation. I wonder if this is a problem that only existed in the old repl where every \n caused a send, and won't be an issue in nrepl, where the entire sent region is a single eval.

emestee commented 11 years ago

@roti please pull the latest version and see if this solves your problem

roti commented 11 years ago

I tested the latest fix, and now it seems that evaluating from a buffer does not happen in the namespace of the buffer, but in the namespace of the REPL. If I understand well, this is the change you made.

However, this is not the problem I reported. My concern is with the fact that evaluating a (ns ...) form from a buffer should also change the current namespace. This is a functionality of the REPL, and it should be already built in the nrepl server. I find it strange that the nrepl client from SublimeREPL somehow changes this behaviour, and the current namespace remains user (or whatever the initial namespace was). In other words, it's not about what namespace is sent along with the form to the nrepl server, but about what the current namespace is after evaluation. So, as far as I am concerned, the problem is not fixed.

Regarding autodetection of buffer namespace, and sending it along with the form to the nrepl server (so that evaluation from buffer happend in the buffer namespace, and not the REPL namespace), I think it's a good feature. It should not be removed. And detection should not happen based on lein conventions, but based on the clojure specifications. I can dig a little bit into this and send you the details, if you want.

emestee commented 11 years ago

The current send-to-eval code is inherited from the previous subprocess-based integration. It tries to autodetect a namespace declaration on the top of the file. If there is one, then the sent code will be evaluated in it, or, if it doesn't exist in the running instance, in the user namespace.

This has some silly side-effects:

Please tell me if the following behaviour makes sense to you:

roti commented 11 years ago

Any evaluated code that begins with (ns ...) or (in-ns ..) will switch the current REPL namespace, regardless of any namespace declaration at the top of the file

Well, yes. This is what these forms do (see ns and in-ns)

If the source buffer has a (ns ...) declaration on top of the file, and that namespace does not exist in the running instance, the declaration is evaluated first to create the namespace, then the code is evaluated in it, and the current namespace is not switched in the REPL. An alternative would be to switch the namespace, but manage a stack of namespaces so that you can easily 'pop' to the previous namespace if the switch was unwanted.

Yes, the namespace should not be switched in the REPL. The reasoning is that the namespace in the REPL should only be switched when I explicitely evaluate (ns ...) or (in-ns ...). Think about the following: you are working in namespace A, which uses namespace L as it's library. Then you want to change something in L so you switch to the L buffer and evaluate your change. Afterwards you switch back to A and continue your work. This is a very common scenario. Alternatively, you could change the current namespace, immediately when I switch the buffer, and I can see that change in the REPL prompt. Regarding what to do when the detected namespace does not exist, I would find it good to be created automatically, so that I don't need to evaluate (ns ...) by hand. But it's a little bit dangerous because it's a hidden evaluation. It may be an unintuitive behaviour, therefore causing confusion. So I would say yes, let's create it automatically, but also put a warning somewhere ("WARNING: namespace not found, creating it") so that I know that my action also caused the creation of that namespace.

Any code sent from a buffer that does not have a (ns ...) declaration on top will be evaluated in the current namespace.

Yes. And "declaration on top" should mean "The first form in the buffer, comments excluded". This is somewhat tricky because having a (ns ...) at the top is pure convention. I can have it in the middle of my code, or can have more than one (ns ...) in a file. But since 99% of clojure code respects this convention it makes sense to have it like this.

An UI option is added to the effect of "Evaluate selection/block in current REPL namespace".

Yes, I was also about to suggest that because it happens often to use the current buffer as scratchpad, just to do some quick things and then delete them from the buffer (you could call this "beeing lazy to open a new empty buffer").

emestee commented 11 years ago

Okay, then I am going to adopt this convention. I'll try to implement this tonight.

So I would say yes, let's create it automatically, but also put a warning somewhere ("WARNING: namespace not found, creating it") so that I know that my action also caused the creation of that namespace.

This relates to another feature that's not been implemented: I can not currently display anything in the REPL buffer that was not generated on the server side (reason being SublimeREPL views the repl integration as a byte stream, and currently Clojure integration simply translates the incoming messages into a byte stream, there is no way to inject something locally so that it can be rendered in the REPL buffer). I will, however, implement it anyway, because I think it's a good idea to display the code that was sent to evaluation, or some locally generated messages such as the one you bring up, perhaps in a shortened form, so that the user can see what was sent to the REPL along with its evaluation results.

emestee commented 11 years ago

Please pull and tell me if this now behaves as intended. There is no UI option for evaluating in current namespace yet. I am also aware of the missing line breaks, it's annoying. I'll fix that too.