Fix shortcut for REPL buffers when type is "any" #2484

kommen · 2018-10-12T21:54:17Z

This fixes a regression from #2467 where in clojure repl buffers the cider eldoc feature stopped working.

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (make test)
All code passes the linter (make lint) which is based on elisp-lint and includes
- byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.

kommen · 2018-10-12T23:05:48Z

Test failures seem to be due to an upstream change: vspinu/sesman#10

bbatsov · 2018-10-13T07:02:15Z

cider-connection.el

@@ -702,7 +702,8 @@ are synonyms.  If ENSURE is non-nil, throw an error if either there is
 no linked session or there is no REPL of TYPE within the current session."
  (if (and (derived-mode-p 'cider-repl-mode)
           (or (null type)
-               (string= cider-repl-type type)))
+               (string= cider-repl-type type)
+               (string= "any" type)))


Hmm, as any and multi can't be synonyms here maybe we should treat them differently by default? It also seems to me that this check should be after the null check, so the code reads better.

On a related note - can you please change these types to be symbols instead of strings everywhere. It's very weird to pass around types as strings in Elisp and I guess this was just some oversight of ours when REPL types were added. For backward compatibility you can just coerce type arguments to symbol in methods like this one (there's a function cider-maybe-symbol or something like this.

Also - if we're using symbols we can use the more effecient eq checks instead of string= and equal.

Hmm, as any and multi can't be synonyms here maybe we should treat them differently by default?

How do you men differently?

I can take care of the rest.

Well, in this particular code snippet passing "multi" will result in the same bug. :-) I'm saying this because in some places we treat "multi" and "any" as the same and somewhere differently. Probably we should agree they are different.

The fix doesn't look quite right to me. This function becomes unreadable with all those if-else and cons.

Any objection against reverting #2467 and just adding (when (equal type "any") (setq type "multi")) as the first line in the function?

Any objection against reverting #2467 and just adding (when (equal type "any") (setq type "multi")) as the first line in the function?

How would this address the original issue #2467 fixed?

I think it'd better to use something like (member type '(any multi)) instead of converting any to multi. As a said - there are cases where semantically they won't be exactly the same, so I'd rather keep them separate.

This would need to be done at least twice in this function and at least once in the utility function. Plus there is already a re-assignment to "multi" in the let part. I If the semantics of this function ever diverges one can modify it later, for now the simplicity of the code is more important. None of us noticed the issue with the original PR.

@kommen
sorry, by reverting I meant reverting the "any" implementation for this function only, not the whole patch

bbatsov · 2018-10-28T07:47:07Z

@kommen Ping :-)

kommen · 2018-10-28T18:07:08Z

@bbatsov I've addressed two of your comments.

Regarding changing the "clj", "cljs", "any" and "multi" repl types from strings to symbols:
I don't think I fully grasp the implications of changing it everywhere, e.g here:

cider/cider.el

Line 1011 in d1eef83

(plist-put :repl-type "clj")

You think it is fine to change it here as well?

bbatsov · 2018-10-28T18:47:48Z

Yep. In the end there should be not strings referring to repl types, only symbols.

kommen · 2018-10-29T07:40:35Z

cider-connection.el

           (scored-repls
            (delq nil
                  (mapcar (lambda (b)
                            (let ((bparams (cider--gather-connect-params nil b)))
-                              (when (eq cljsp (string-match-p "cljs" (plist-get bparams :repl-type)))
+                              (when (eq cljsp (string-match-p "cljs"


Anybody knows what cljsp is supposed to be about? I assume it is to consider pending-cljs repls to be reused by cljs repls? But not sure.

It's non-nil when it's either cljs or pending-cljs.

kommen · 2018-10-29T07:42:35Z

@bbatsov I've pushed the current state of my branch. Things still needed:

figure out which functions need to be made backwards compatible by accepting strings and turn them into symbols with cider-maybe-intern
ensure fec7691#r228822378 is doing the right thing
investigate ci failure
update doc strings
add changelog

Not sure I will have time this week to address these though.

kommen · 2018-11-03T01:46:31Z

This is ready for review.

bbatsov · 2018-11-06T10:22:28Z

@kommen The code looks good. You just need to squash together all commits related to the transition from strings to symbols.

bbatsov · 2018-11-06T10:22:45Z

CHANGELOG.md

+
+### Changes
+
+* [#2484](https://github.com/clojure-emacs/cider/pull/2484): REPL types are now symbols instead of strings


You forgot the . here.

kommen · 2018-11-06T10:28:24Z

Thanks @bbatsov! I'll test this branch in day to day work somewhen this week to be more confident it doesn't break anything and then will address your comments.

bbatsov · 2018-11-06T10:35:56Z

Sounds like a plan to me!

bbatsov · 2018-11-16T06:39:20Z

Is the PR ready to go?

kommen · 2018-11-16T06:51:24Z

using it the branch for day to day stuff i found some more issues stilll. i’d rather test it a bit more.

kommen · 2018-11-16T06:53:02Z

but i don’t know of any remaining issues right now

bbatsov · 2018-11-16T11:44:46Z

Well, if you work it day-to-day I guess it doesn't break anything major. :-) Anyways, I'm fine with you testing it longer if you prefer that.

Make public functions backwards compatible by converting type string to symbol

kommen · 2018-11-18T16:57:22Z

@bbatsov I've squashed the commits to combine them logically.

As pauld on the slack also tried the branch and confirmed it fixed an issue for him, I think this should be good to merge.

bbatsov · 2018-11-18T17:37:01Z

Thanks!

arichiardi · 2018-11-21T03:52:05Z

Awesome job folks I have just bumped into this and wanted to say thanks!

bbatsov reviewed Oct 13, 2018

View reviewed changes

kommen force-pushed the fix-current-repl-shortcut branch from 1d7a634 to b21eb37 Compare October 28, 2018 18:02

kommen commented Oct 29, 2018

View reviewed changes

bbatsov reviewed Nov 6, 2018

View reviewed changes

kommen added 2 commits November 15, 2018 17:47

Fix shortcut for REPL buffers when type is "any"

3645583

Don't treat "multi" and "any" reply types as synoyms

a4b928e

kommen force-pushed the fix-current-repl-shortcut branch from 48c8531 to c06d707 Compare November 15, 2018 16:48

kommen mentioned this pull request Nov 15, 2018

cider-connect-clj&cljs results in no clj repl connected #2519

Closed

kommen force-pushed the fix-current-repl-shortcut branch from f3726df to 1b3509a Compare November 17, 2018 19:27

kommen added 2 commits November 17, 2018 20:35

Use symbols for repl types instead of strings and efficient comparisons

b079aeb

Make public functions backwards compatible by converting type string to symbol

Simplify cljs reuse check

085d013

kommen force-pushed the fix-current-repl-shortcut branch from 1b3509a to 085d013 Compare November 17, 2018 19:35

bbatsov merged commit 1fd1275 into clojure-emacs:master Nov 18, 2018

kommen mentioned this pull request Nov 19, 2018

:select nil in popup-rules for cider REPL buffer causes error on cider-jack-in-cljs #2515

Closed

mallt mentioned this pull request Nov 19, 2018

State middleware not detecting correct repl type #2456

Closed

kommen deleted the fix-current-repl-shortcut branch November 19, 2018 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix shortcut for REPL buffers when type is "any" #2484

Fix shortcut for REPL buffers when type is "any" #2484

kommen commented Oct 12, 2018 •

edited

Loading

kommen commented Oct 12, 2018

bbatsov Oct 13, 2018

bbatsov Oct 13, 2018

kommen Oct 13, 2018

bbatsov Oct 13, 2018

vspinu Oct 13, 2018

kommen Oct 15, 2018

bbatsov Oct 15, 2018

vspinu Oct 15, 2018

bbatsov commented Oct 28, 2018

kommen commented Oct 28, 2018

bbatsov commented Oct 28, 2018

kommen Oct 29, 2018

vspinu Oct 29, 2018

kommen commented Oct 29, 2018 •

edited

Loading

kommen commented Nov 3, 2018

bbatsov commented Nov 6, 2018

bbatsov Nov 6, 2018

kommen commented Nov 6, 2018

bbatsov commented Nov 6, 2018

bbatsov commented Nov 16, 2018

kommen commented Nov 16, 2018

kommen commented Nov 16, 2018

bbatsov commented Nov 16, 2018

kommen commented Nov 18, 2018

bbatsov commented Nov 18, 2018

arichiardi commented Nov 21, 2018


		### Changes

		* [#2484](https://github.com/clojure-emacs/cider/pull/2484): REPL types are now symbols instead of strings

Fix shortcut for REPL buffers when type is "any" #2484

Fix shortcut for REPL buffers when type is "any" #2484

Conversation

kommen commented Oct 12, 2018 • edited Loading

kommen commented Oct 12, 2018

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 Oct 28, 2018

kommen commented Oct 28, 2018

bbatsov commented Oct 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommen commented Oct 29, 2018 • edited Loading

kommen commented Nov 3, 2018

bbatsov commented Nov 6, 2018

Choose a reason for hiding this comment

kommen commented Nov 6, 2018

bbatsov commented Nov 6, 2018

bbatsov commented Nov 16, 2018

kommen commented Nov 16, 2018

kommen commented Nov 16, 2018

bbatsov commented Nov 16, 2018

kommen commented Nov 18, 2018

bbatsov commented Nov 18, 2018

arichiardi commented Nov 21, 2018

kommen commented Oct 12, 2018 •

edited

Loading

kommen commented Oct 29, 2018 •

edited

Loading