Support REPL flavors and Lumo #44

arichiardi · 2017-03-01T20:52:34Z

This patch adds the inf-clojure-repl-flavor defcustom in order to
support different kind of configurations based on the selected flavor.
The default flavor is 'clojure, and the other supported one is 'lumo.
It also adds defcustoms to further customize hostname, port and
classpath used in the Lumo flavor.

Customized:

inf-clojure-var-doc-form-lumo
inf-clojure-completion-form-lumo -> returns a `#js tag literal, maybe some more customization will be needed

The following don't need customizations:

inf-clojure-set-ns-form
inf-clojure-macroexpand-form
inf-clojure-macroexpand-1-form

TODOs

(setq inf-clojure-var-source-form "(lumo.repl/source %s)") - Add lumo.repl/source anmonteiro/lumo#84
(setq inf-clojure-ns-vars-form "(lumo.repl/dir %s)") - Add lumo.repl/dir anmonteiro/lumo#87
(setq inf-clojure-var-apropos-form "(lumo.repl/apropos %s)") - Add lumo.repl/apropos anmonteiro/lumo#86
(setq inf-clojure-arglist-form "(lumo.repl/get-arglists \"%s\")") - Add lumo.repl/get-arglists anmonteiro/lumo#88

arichiardi · 2017-03-01T20:57:16Z

inf-clojure.el


+(defgroup lumo nil


Probably I don't need this one, I will delete it.

arichiardi · 2017-03-01T20:57:51Z

inf-clojure.el

+
+;; AR - TODO Alternatively you can specify a command string that will be called,
+;; which should return a string.
+(defcustom inf-clojure-lumo-classpath-generator "cp"


This name is arbitrary, but it is kind of standard to call it something like this.

arichiardi · 2017-03-01T20:59:08Z

inf-clojure.el

+
+(defcustom inf-clojure-lumo-port ""
+  "The port used to launch lumo.
+Note that if port is specified, inf-clojure will connect to it by


Will rewrite this as well.

arichiardi · 2017-03-02T02:26:13Z

inf-clojure.el

+
+;; AR - TODO Alternatively you can specify a command string that will be called,
+;; which should return a string.
+(defcustom inf-clojure-lumo-classpath-generator "cp"


This approach could be extended to all REPL flavors, they all need a classpath to be built if using the "command" approach. Socket of course will already have everything setup. Lumo does not differ in this aspect. I personally settled on the socket connection but it is handy sometimes to start things in place.

arichiardi · 2017-03-02T02:27:48Z

inf-clojure.el

+
+(defun inf-clojure--lumo-program ()
+  "Return inf-clojure-program for lumo."
+  (if (inf-clojure--empty-string-p inf-clojure-lumo-port)


an explicit command is returned only if the port is empty, we should probably refine this

bbatsov · 2017-03-02T06:44:02Z

inf-clojure.el

@@ -559,6 +575,26 @@ The value is nil if it can't find one."
  "Return the name of the symbol at point, otherwise nil."
  (or (thing-at-point 'symbol) ""))

+(defun inf-clojure--empty-string-p (string)


Isn't there such a function in subr-x.el?

Don't know what it is, but I will check..

bbatsov · 2017-03-02T06:45:14Z

inf-clojure.el

@@ -208,6 +209,21 @@ whichever process buffer you want to use.")

 (put 'inf-clojure-mode 'mode-class 'special)

+(defcustom inf-clojure-repl-flavor 'clojure


I really think there should be only a default REPL type and some command that let's easily pick the type of REPL you want to create. Otherwise creating both a lumo repl and a clojure repl would be really frustrating.

So I am beginning to understand a bit more, a defcustom is global and therefore this will be set once and forever right? So this becomes buffer-local? I need to read more on this...and thanks for reviewing!

Basically such variable such be declared as defvar-local instead of as defcustoms. There should probably be some defcustoms with the values for the different types of repls, but the actual thing that would be set in the code would be a local variable that's specific to the REPL. You can see in CIDER's code it makes heavy use of local variables.

Ok I was reading a bit more. By defcustom with the types you mean a defcustom that uses :set to set the local var? The only thing I am missing is how to set the buffer local from a defcustom. I am checking also cider.

bbatsov · 2017-03-02T06:46:01Z

inf-clojure.el

+
+  ;; Uncomment after https://github.com/anmonteiro/lumo/pull/88
+  ;; (setq inf-clojure-arglist-command "(lumo.repl/get-arglists \"%s\")")
+  (setq inf-clojure-var-doc-command "(lumo.repl/doc %s)")


Modifying defcustoms like this is a bad idea in general. We should make those variables buffer-local so that changing them for one REPL won't affect the rest.

And this is were I tell you that I am not an expert at all here, so please guide me on the best approach. For example how to set those defcustoms then? Should those be buffer-local instead?

Fixed this in the other commit (which of course we can squash when things are good)

You should be setting and checking only buffer-locals. The defcustoms should simply exist as the values of the different flavours. This means you'll have one doc buffer-local and two defcustoms - one for clojure and one for lumo. Alternatively you can just have a buffer-local and hardcode the defcustom values, but this will make the code less flexible, so I wouldn't recommend it.

Oh I was afraid of this 😄 this means converting all the defcustoms I am using right?

On second thought, why are we exposing something like inf-clojure-var-doc-command anyways? It is kind of an implementation detail that we probably should hide from user customization and therefore make it defvar-local.

As you case see above, a lot of those defcustoms need to be set to something else in case of lumo flavor, and will need to be changed again in case of planck flavor and "normal cljs" repl. If we do like you are suggesting it would mean 1 defvar-local + 4 defcustom already for each var that needs customization...It might become unmanageable.

Whether those are defcustoms are hardcoded strings, the complexity of the code won't be changed that much. The advantage of using defcustoms is that users might be able to use some of the features on "unsupported" repls if they have the desire to customize the defcustoms. Or they can tweak the behaviour of something - e.g. use some different completion code. As I said - I can live with the code snippets for the various repls being hardcoded as well - it makes little difference to me.

OK then, I will begin the big refactoring soon-ish, just wanted to be sure it is what you were thinking 👍

bbatsov · 2017-03-02T06:47:36Z

inf-clojure.el

+  "Return true if the lumo is the target REPL."
+  (comint-send-string (inf-clojure-proc) "(js/global.hasOwnProperty \"$$LUMO_GLOBALS\")"))
+
+(defun inf-clojure--lumo-program ()


I think this name is a bit confusing. This doesn't return the lumo program, but some command complete with parameters.

Yeah, I'll change it

bbatsov · 2017-03-02T06:48:25Z

inf-clojure.el

+     ((message (concat "Option \"" classpath-option "\" was not a (readable) file, the classpath will be empty."))
+      ""))))
+
+(defun inf-clojure--maybe-set-program (program)


I don't quite get the point of this.

I added it because I though you'd want to modify the inf-clojure-program only if it is set as a command , not if it is a cons cell, which indicates you don't want to launch anything...open to other ideas..

I will remove this.

bbatsov · 2017-03-02T06:50:20Z

inf-clojure.el

+(defun inf-clojure--flavor-setup ()
+  "Setup inf-clojure defcustoms depending on the choose flavor."
+  (pcase inf-clojure-repl-flavor
+    (lumo (progn (message "[inf-clojure] will switch to the Lumo flavor")


Those messages are bit confusing as you won't really be switching to anything - you'll start a REPL of this type.

True, it was part of my first attempt

bbatsov · 2017-03-02T08:47:09Z

Btw, seems we can support planck properly using the same technique.

bbatsov · 2017-03-02T08:49:13Z

In case you're wondering - #34 reminded me of this. Seems planck also needs different code for completion, ns switching and so on.

arichiardi · 2017-03-05T18:16:07Z

inf-clojure.el

-    ("lein" inf-clojure-lein-cmd)
-    ("boot" inf-clojure-boot-cmd)
-    (_ inf-clojure-generic-cmd)))
+  (if-let ((flavor-command (inf-clojure-flavor)))


@bbatsov this is what I meant basically, if inf-clojure-repl-flavor is present, it takes precedence. It is either this is coming up with a project marker of our own, which I don't like that much.

arichiardi · 2017-03-05T18:19:55Z

inf-clojure.el

+  "Determine the flavor of the project."
+  (pcase inf-clojure-repl-flavor
+    (lumo (inf-clojure--lumo-repl-command))
+    (_ nil)))


Many cases here, if I had to sketch a Clojure data structure, I would do:

{:project-types #{:boot :lein} :project-flavors #{:clj :lumo :planck :cljs}}

Just want to make sure we are on the same page 👍

The point is that you can't really detect this, you can only set it via some dir-local for instance. That's why I'd rather simply leave people to configure how to start the REPL, run some eval after the REPL starts to check what's its type and use the result to set its flavour.

Alternatively there could be some commands like inf-clojure-start-lumo and so on, but I generally prefer to avoid having an excessive amount of commands.

So, I assume people would simply change inf-clojure-lein-cmd or whatever to a command starting a lumo repl if they want to do this.

Are you talking about the start command specifically or all the commands to customize?
Because it is a bit inconvenient imho to carry around a huge .dir-locals.el from project to project for instance, the local var seems like a good compromise, let me send an addition I made.

EDIT: scratch that, you next comment clarifies.

arichiardi · 2017-03-05T20:12:50Z

inf-clojure.el

+  "Lumo command to query inferior Clojure for a var's documentation."
+  :type 'string)
+
+(defun inf-clojure-var-doc-form ()


So I hope I got this right, I have used form instead of command as it is more appropriate, but ideas on naming are always welcome.

I'm fine with the naming. It's better indeed.

bbatsov · 2017-03-06T06:44:06Z

I have no idea what those character are. I thought you tested this in the past and it was working ok then.

arichiardi · 2017-03-06T07:02:32Z

Ok cleaned up the characters as well. At the moment it does the cleaning even when in other flavors, because I didn't have time to check how to set the buffer local var when launching the *inf-clojure* buffer. Anyways, I think that is a small fix for more expert eyes. It is in the last commit (dc63105
). I have learned more about Emacs today than in my whole life plus previous lives 😄

bbatsov · 2017-03-06T09:15:02Z

inf-clojure.el

+      (set-process-filter proc prev-filter))
+    is-lumo))
+
+;; The defcustoms for the following are already ok:


Not sure if these comments belong here.

Yeah probably they are issues to open

bbatsov · 2017-03-06T09:15:46Z

inf-clojure.el


+(defun inf-clojure--lumo-mode-p (proc)


I'd make this a generic function that takes as extra params some form and some expected result, so we could reuse it for other flavours.

Add also change the name to inf-clojure--lumo-p.

bbatsov · 2017-03-06T09:16:05Z

inf-clojure.el

+;;;; Lumo
+;;;; ====
+
+;; When we'll have tests, this is one


Guess you can remove this as the subsequent function actually tests for it.

bbatsov · 2017-03-06T09:16:51Z

inf-clojure.el

@@ -526,6 +567,19 @@ The prefix argument SWITCH-TO-REPL controls whether to switch to REPL after the
  "Command to query inferior Clojure for a var's documentation."
  :type 'string)

+(defcustom inf-clojure-var-doc-command-lumo


command -> form

bbatsov · 2017-03-06T09:17:02Z

inf-clojure.el

@@ -526,6 +567,19 @@ The prefix argument SWITCH-TO-REPL controls whether to switch to REPL after the
  "Command to query inferior Clojure for a var's documentation."
  :type 'string)

+(defcustom inf-clojure-var-doc-command-lumo
+  "(lumo.repl/doc %s)\n"
+  "Lumo command to query inferior Clojure for a var's documentation."


bbatsov · 2017-03-06T09:18:08Z

inf-clojure.el

+(defun inf-clojure-var-doc-form ()
+  "Return the form to query inferior Clojure for a var's documentation.
+If you are using flavors, it will pickup the most approapriate
+`inf-clojure-var-doc-command` variant."


command -> form

bbatsov · 2017-03-06T09:18:49Z

inf-clojure.el

 of command, consisting of a host and port
 number (e.g. (\"localhost\" . 5555)).  That's useful if you're
 often connecting to a remote REPL process."
  :type '(choice (string)
                 (cons string integer)))

+(defvar-local inf-clojure-repl-flavor nil


Btw, probably we should simply use type instead of flavor. Seems more obvious to me.

It is maybe better, I so like flavor I will need to find an open source project where to use it 😀 Only Gradle was so bold as to actually add it as concept 😁

bbatsov · 2017-03-06T09:20:00Z

inf-clojure.el

+
+(defun inf-clojure--set-flavor (proc)
+  "Set the flavor if has not already been set."
+  (when (not inf-clojure-repl-flavor)


when + not = unless :-)

Right! Emacs has that 😀

bbatsov · 2017-03-06T09:20:16Z

inf-clojure.el

+Its root binding is nil and it can be further customized using
+either `setq-local` or an entry in `.dir-locals.el`." )
+
+(defun inf-clojure--probe-flavor (proc)


detect type perhaps?

bbatsov · 2017-03-06T09:21:16Z

The PR is in pretty good shape already. I've added a few small remarks and that's pretty much all.

You'll also have to update the changelog and the README - maybe you can add a small section on lumo there.

arichiardi · 2017-03-06T18:13:45Z

@bbatsov I renamed all of the vars, is it ok or you prefer another PR for that?

arichiardi · 2017-03-06T18:27:53Z

Ok I made the require changes, REPL type is in. The renaming of the commands to -forms is not in here anymore, I will open another PR once this is in.

This patch adds the `inf-clojure-repl-type` defcustom in order to support different kind of configurations based on the selected type. The default is `'clojure`, and the other supported one is `'lumo`.

The change deletes some arbitrary characters that the Lumo REPL emits.

bbatsov · 2017-03-06T19:56:11Z

OK, guess you can update the changelog and the README in the second PR.

Btw, if you have any planck familiarity it'd be great to add support for it as well. Now this should be trivial.

arichiardi · 2017-03-11T17:13:59Z

There is still a case when the repl is not detected, I have a patch already but I want to try it thoroughly

arichiardi force-pushed the lumo-and-flavors branch from 77b685b to fba370e Compare March 1, 2017 20:53

arichiardi changed the title ~~Support Repl flavors and Lumo~~ Support REPL flavors and Lumo Mar 1, 2017

arichiardi commented Mar 1, 2017

View reviewed changes

inf-clojure.el Outdated

(defgroup lumo nil

Copy link

Contributor Author

arichiardi Mar 1, 2017

Choose a reason for hiding this comment

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

Probably I don't need this one, I will delete it.

arichiardi commented Mar 1, 2017

View reviewed changes

arichiardi force-pushed the lumo-and-flavors branch 3 times, most recently from 66f5677 to 1518053 Compare March 2, 2017 02:22

arichiardi commented Mar 2, 2017

View reviewed changes

arichiardi force-pushed the lumo-and-flavors branch from 1518053 to f0ae1df Compare March 2, 2017 02:47

bbatsov reviewed Mar 2, 2017

View reviewed changes

bbatsov mentioned this pull request Mar 2, 2017

Accept cons pair for tcp connections. #34

Merged

arichiardi force-pushed the lumo-and-flavors branch 3 times, most recently from b9c958f to 7ecd990 Compare March 4, 2017 00:25

bbatsov mentioned this pull request Mar 4, 2017

Company: An error occurred in auto-begin #36

Closed

arichiardi force-pushed the lumo-and-flavors branch from 7ecd990 to e14ad28 Compare March 5, 2017 18:14

arichiardi commented Mar 5, 2017

View reviewed changes

arichiardi force-pushed the lumo-and-flavors branch 2 times, most recently from 903503b to 4115803 Compare March 5, 2017 20:12

arichiardi commented Mar 5, 2017

View reviewed changes

arichiardi force-pushed the lumo-and-flavors branch from be4ddf0 to dc63105 Compare March 6, 2017 06:58

bbatsov reviewed Mar 6, 2017

View reviewed changes

arichiardi force-pushed the lumo-and-flavors branch from dc63105 to d2e19f6 Compare March 6, 2017 18:02

arichiardi force-pushed the lumo-and-flavors branch 2 times, most recently from 1fe8d24 to 2d188de Compare March 6, 2017 18:26

arichiardi added 3 commits March 6, 2017 11:15

Support REPL types and Lumo

cb790d0

This patch adds the `inf-clojure-repl-type` defcustom in order to support different kind of configurations based on the selected type. The default is `'clojure`, and the other supported one is `'lumo`.

Fix typo in inf-clojure-*-cmd defcustoms

71117d5

Remove unwanted ansi chars from the REPL output

32b39df

The change deletes some arbitrary characters that the Lumo REPL emits.

arichiardi force-pushed the lumo-and-flavors branch from 2d188de to 32b39df Compare March 6, 2017 19:16

bbatsov merged commit a36d2a0 into clojure-emacs:master Mar 6, 2017

arichiardi deleted the lumo-and-flavors branch March 6, 2017 20:37

This was referenced Mar 6, 2017

Modify CHANGELOG and README for REPL types #49

Merged

Lumo mode #43

Closed

bbatsov mentioned this pull request Mar 9, 2017

Add possibility to customize more vars #32

Merged

bbatsov mentioned this pull request Mar 12, 2017

Add special handling for planck REPLs #56

Closed

		@@ -208,6 +209,21 @@ whichever process buffer you want to use.")

		(put 'inf-clojure-mode 'mode-class 'special)

		(defcustom inf-clojure-repl-flavor 'clojure

Support REPL flavors and Lumo #44

Support REPL flavors and Lumo #44

Conversation

arichiardi commented Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

arichiardi Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichiardi Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

arichiardi Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichiardi Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Mar 2, 2017

bbatsov commented Mar 2, 2017

Choose a reason for hiding this comment

arichiardi Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichiardi Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

arichiardi Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Mar 6, 2017

arichiardi commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Mar 6, 2017

arichiardi commented Mar 6, 2017 • edited Loading

arichiardi commented Mar 6, 2017

bbatsov commented Mar 6, 2017

arichiardi commented Mar 11, 2017

arichiardi commented Mar 1, 2017 •

edited

Loading

arichiardi Mar 1, 2017 •

edited

Loading

arichiardi Mar 2, 2017 •

edited

Loading

arichiardi Mar 2, 2017 •

edited

Loading

arichiardi Mar 3, 2017 •

edited

Loading

arichiardi Mar 5, 2017 •

edited

Loading

arichiardi Mar 5, 2017 •

edited

Loading

arichiardi Mar 5, 2017 •

edited

Loading

arichiardi commented Mar 6, 2017 •

edited

Loading