Add inf-clojure-prompt-on-commands and closes #48 #51

arichiardi · 2017-03-08T23:50:38Z

The patch introduces a way to globally disable prompting. It deletes
inf-clojure-prompt-when-set-ns.

The commits are consistent with our [contribution guidelines][1]
The new code is not generating bytecode or M-x checkdoc warnings
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the README

arichiardi · 2017-03-08T23:53:47Z

Actually this does not make proper sense in terms of usability, apropos for instance has to prompt I guess. Maybe it is better if we come up with a list of commands that need to prompt for input.

arichiardi · 2017-03-09T00:11:23Z

On second though I would probably always prompt for apropos and that is it...even better would some to invert the behavior of inf-clojure-prompt-on-commands with a C-u prefix

bbatsov · 2017-03-09T07:24:36Z

Actually this does not make proper sense in terms of usability, apropos for instance has to prompt I guess. Maybe it is better if we come up with a list of commands that need to prompt for input.

Yeah, appropos shouldn't operate at all on this symbol at point. Guess we should emulate the cider behaviour for it.

On second though I would probably always prompt for apropos and that is it...even better would some to invert the behavior of inf-clojure-prompt-on-commands with a C-u prefix

Yeah, it'd be best to add prefix arg support for all commands can prompt or not.

bbatsov · 2017-03-09T08:57:34Z

You'll have to rebase this, btw.

arichiardi · 2017-03-09T16:45:23Z

Don't know how you understood the latter msg, it looks like my fingers did not want to write some words 😀

arichiardi · 2017-03-09T20:04:51Z

Is inf-clojure-arglist supposed to prompt? It seems to me that it should not, as it is only called by eldoc and it is guarded by the inf-clojure-show-arglist command. Just wanted to confirm.

bbatsov · 2017-03-09T20:38:09Z

Is inf-clojure-arglist supposed to prompt? It seems to me that it should not, as it is only called by eldoc and it is guarded by the inf-clojure-show-arglist command. Just wanted to confirm.

This command can probably be removed. It was added before the mode had eldoc support as a simple alternative to it. Seems pretty useless to me right now.

arichiardi · 2017-03-09T20:39:41Z

Ok I will remove it

bbatsov · 2017-03-09T22:49:48Z

inf-clojure.el

-  (interactive (inf-clojure-symprompt "Var doc" (inf-clojure-var-at-pt)))
-  (comint-proc-query (inf-clojure-proc) (format (inf-clojure-var-doc-form) var)))
+  (interactive "P")
+  (let ((var (if (inf-clojure--should-prompt-on-commands var)


Seems to me you're not using your helper function correctly. I assume it was intended to be used with some prefix argument.

Can you expand on this? It is working fine here, prompting when the function returns true.

still valid, sorry for the mess 😉

bbatsov · 2017-03-09T22:51:04Z

inf-clojure.el

+  :package-version '(inf-clojure . "2.0.0")
+  :safe 'booleanp)
+
+(defun inf-clojure--should-prompt-on-commands (&optional invert)


prompt-on-commands-p

bbatsov · 2017-03-09T22:52:12Z

inf-clojure.el

@@ -238,11 +238,17 @@ This should usually be a combination of `inf-clojure-prompt' and
 `inf-clojure-subprompt'."
  :type 'regexp)

-(defcustom inf-clojure-prompt-on-set-ns t
+(defcustom inf-clojure-prompt-on-commands t


That's a bit vague. Maybe "prompt-for-symbol-at-point" or something. And you forgot to update the docstring.

the new name would require to flip the logic and rename also the other functions...also maybe in the future not all the commands will take symbol at point so I was wondering if we could leave it like this.

"like this" meaning what?

Meaning inf-clojure-prompt-on-commands

I can live with this.

Ok let me know if this requires more work

bbatsov · 2017-03-10T06:34:02Z

inf-clojure.el

+  "Send a command to the inferior Clojure to give source for VAR.
+See variable `inf-clojure-var-source-command'."
+  (interactive "P")
+  (let ((var (if (inf-clojure--prompt-on-commands-p var)


You should add the prefix argument to the function signature and pass it here, instead of var.

Isn't that just a name (symbol) binding? Btw there is the same thing in cider. I mean, it is working this way, maybe there is a better way I am not getting...Can you show me a simple piece of code?

cider uses the magic current-prefix-arg variable for this.

(defun cider--find-dwim-interactive (prompt) "Get interactive arguments for jump-to functions using PROMPT as needed." (if (cider--prompt-for-symbol-p current-prefix-arg) (list (cider-read-from-minibuffer prompt (thing-at-point 'filename))) (list (or (thing-at-point 'filename) "")))) ; No prompt. (defun cider--invert-prefix-arg (arg) "Invert the effect of prefix value ARG on `cider-prompt-for-symbol'. This function preserves the `other-window' meaning of ARG." (let ((narg (prefix-numeric-value arg))) (pcase narg (16 -1) ; empty empty -> - (-1 16) ; - -> empty empty (4 nil) ; empty -> no-prefix (_ 4)))) ; no-prefix -> empty (defun cider--prefix-invert-prompt-p (arg) "Test prefix value ARG for its effect on `cider-prompt-for-symbol`." (let ((narg (prefix-numeric-value arg))) (pcase narg (16 t) ; empty empty (4 t) ; empty (_ nil)))) (defun cider--prompt-for-symbol-p (&optional prefix) "Check if cider should prompt for symbol. Tests againsts PREFIX and the value of `cider-prompt-for-symbol'. Invert meaning of `cider-prompt-for-symbol' if PREFIX indicates it should be." (if (cider--prefix-invert-prompt-p prefix) (not cider-prompt-for-symbol) cider-prompt-for-symbol))

Btw, for consistency you can use the same defcustom name - inf-clojure-prompt-for-symbol.

One more thing - by moving out the var init from interactive you're going to break those functions when invoked without parameters. I'm guessing you didn't really test this code. :-) You'll have to make params like var optional and prompt for them only when they are not passed explicitly (or remove them completely).

I will paste a gif showing it OK? If I hadn't tested it I would not even have opened a PR 😀 maybe I did not test some case, but all the interactive calls are working fine this way. Do you mean a call with no params from ielm or call with no params on M-x. The latter works but I will confirm it again today.

Also, here is the excerpt I copied from cider: https://github.com/clojure-emacs/cider/blob/1ce1d9998fc756d3dbbb491cc6f4f0c4c397b450/cider-doc.el#L208

Read carefully the code you copied - arg there is the prefix arg, not the the symbol at point (like in your case). I guess what you did works because when you removed the var related code from interactive there are two bindings for var within the code, but this is definitely wrong. If you want to do it this way you should rename var to prompt-for-symbl or something.

Maybe I am used to Clojure were you cannot have two bindings at the same time, the first var symbol is bound to the arg, the second is in the let scope and is bound to the result of the function call. In Clojure you take advantage of this in order to override and replace the value in input with the one you calculate and want to use in the scope of the let.

I don't know emacs' guts enough to know its different semantics about var binding and it might be confusing so I think I am going to rename it.

Sorry to bother on this again, I am reading this and I really don't see any difference with Clojure and no reason why the above is wrong:

A variable can have more than one local binding at a time (e.g., if there are nested let forms that bind the variable). The current binding is the local binding that is actually in effect. It determines the value returned by evaluating the variable symbol, and it is the binding acted on by setq.

Again, you ae the boss so I will rename it 😀

bbatsov · 2017-03-10T16:57:52Z

inf-clojure.el

-  (interactive (inf-clojure-symprompt "Var doc" (inf-clojure-var-at-pt)))
-  (comint-proc-query (inf-clojure-proc) (format (inf-clojure-var-doc-form) var)))
+  (interactive "P")
+  (let ((var (if (inf-clojure--prompt-on-commands-p var)


You're killing me, buddy. :-) var is a prefix arg at this point, but after the if returns it's overridden - it becomes the symbol at point. I assumed this was apparent. I find this extremely confusing, so I'd change the parameter name to something else, the let-binding is fine. Hopefully, I managed to make myself clear this time around.

Sorry bro, I am learning right? 😀 Yeah no it was clear, I am going to change that and also use the more robust invert you posted from cider above.

arichiardi · 2017-03-10T17:54:30Z

Ok done, waiting for you further remarks.

bbatsov · 2017-03-10T18:13:02Z

Btw, I meant you should change the parameter names to something invert-prompting or whatever, there was no need to change the let-binding names. You should also mention in each interactive command what does this prefix argument to do.

bbatsov · 2017-03-10T18:16:11Z

inf-clojure.el

+  :package-version '(inf-clojure . "2.0.0")
+  :safe 'booleanp)
+
+(defun inf-clojure--prompt-on-commands-p (&optional arg)


Isn't copying this an overkill? It was needed in CIDER, because some commands had prefix arguments of their own, but here it seems pointless to make it this complex.

Frankly I wonder if we even need this defcustom we can make all the commands not prompt by default and to prompt with a prefix argument (or when there's nothing at point). It would be simple and would get the job done.

Agree on this! I will revert back to the previous version and abolish the defcustom.

bbatsov · 2017-03-10T23:39:44Z

inf-clojure.el

+
+(defun inf-clojure-show-var-source (prompt-for-symbol)
+  "Send a command to the inferior Clojure to give source for VAR.
+See variable `inf-clojure-var-source-form', with non-nil


I'd use a separate sentence for the prefix arg everywhere.

When invoked with a prefix argument prompt-for-symbol....

The patch makes so that the REPL commands don't prompt for things anymore (except for `inf-clojure-apropos`). For prompting, the user will need to pass a prefix argument to the command.

arichiardi force-pushed the no-prompt-global branch from 02b6200 to adafc09 Compare March 8, 2017 23:52

arichiardi force-pushed the no-prompt-global branch 2 times, most recently from b4c8f96 to ea0c5d1 Compare March 9, 2017 22:28

bbatsov reviewed Mar 9, 2017

View reviewed changes

arichiardi force-pushed the no-prompt-global branch from ea0c5d1 to 5250f74 Compare March 9, 2017 23:06

bbatsov reviewed Mar 10, 2017

View reviewed changes

arichiardi force-pushed the no-prompt-global branch from 5250f74 to 518488c Compare March 10, 2017 17:50

bbatsov reviewed Mar 10, 2017

View reviewed changes

arichiardi force-pushed the no-prompt-global branch 2 times, most recently from 082c076 to c0eb05e Compare March 10, 2017 19:09

bbatsov reviewed Mar 10, 2017

View reviewed changes

Commands don't prompt anymore by default closes clojure-emacs#48

ee78b65

The patch makes so that the REPL commands don't prompt for things anymore (except for `inf-clojure-apropos`). For prompting, the user will need to pass a prefix argument to the command.

arichiardi force-pushed the no-prompt-global branch from c0eb05e to ee78b65 Compare March 10, 2017 23:49

bbatsov merged commit d81e266 into clojure-emacs:master Mar 11, 2017

arichiardi deleted the no-prompt-global branch March 11, 2017 17:12

Add inf-clojure-prompt-on-commands and closes #48 #51

Add inf-clojure-prompt-on-commands and closes #48 #51

Conversation

arichiardi commented Mar 8, 2017 • edited Loading

arichiardi commented Mar 8, 2017

arichiardi commented Mar 9, 2017

bbatsov commented Mar 9, 2017

bbatsov commented Mar 9, 2017

arichiardi commented Mar 9, 2017

arichiardi commented Mar 9, 2017 • edited Loading

bbatsov commented Mar 9, 2017

arichiardi commented Mar 9, 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

arichiardi Mar 10, 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

arichiardi Mar 10, 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

arichiardi commented Mar 10, 2017

bbatsov commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichiardi commented Mar 8, 2017 •

edited

Loading

arichiardi commented Mar 9, 2017 •

edited

Loading

arichiardi Mar 10, 2017 •

edited

Loading

arichiardi Mar 10, 2017 •

edited

Loading