-
-
Notifications
You must be signed in to change notification settings - Fork 645
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Also match friendly sessions based on the buffer's ns form #3424
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff might be a little confusing, but luckily, it boils down to two additions that I've marked as multi-line comments.
@@ -615,64 +615,6 @@ REPL defaults to the current REPL." | |||
|
|||
(declare-function cider-classpath-entries "cider-client") | |||
|
|||
(defun cider--sesman-friendly-session-p (session &optional debug) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the new impl uses misc variables/defuns like cider-repl-ns-cache, cider-sync-request:ns-list, I had to move it, keeping the linter happy.
tbh it makes sense to me that if this code now uses more repl-based capabilities, it's defined in cider-repl.el.
An alternative possibly being declare-function
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of declare-function
, so let's keep it moved.
(ns-list (or (process-get proc :all-namespaces) | ||
(let ((ns-list (with-current-buffer repl | ||
(cider-sync-request:ns-list)))) | ||
(process-put proc :all-namespaces ns-list) | ||
ns-list))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is new.
(when-let ((ns (condition-case nil | ||
(substring-no-properties (cider-current-ns :no-default)) | ||
(error nil)))) | ||
;; if the ns form matches with a ns of all runtime namespaces, we can consider the buffer to match | ||
;; (this is a bit lax, but also quite useful) | ||
(with-current-buffer repl | ||
(or (when cider-repl-ns-cache ;; may be nil on repl startup | ||
(member ns (nrepl-dict-keys cider-repl-ns-cache))) | ||
(member ns ns-list)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. At some point it'd be nice to comment all the cases for a friendly session as the function is now huge and it's kind of hard to follow.
Looks good to me now. |
Sample use case:
No linked CIDER sessions
), because the ns form won't match with Haystack's ns listThe "ns list" is basically:
(all-ns)
, computed oncecider-repl-ns-cache
, i.e. the changed namespaces as conveyed bytrack-state
.Cheers - V