Skip to content
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

Enhance cider-connect to show all nREPLs available ports, instead of only Leiningen ones #3399

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Aug 10, 2023

Fixes #3390

Rehash of #3392 with some improvements directly applied, while preserving attribution:

  • Do more processing in Elisp (vs awk)
  • For each 'stragegy' (lein processes, non-lein-process, local, etc), a list of <dir, port> pairs is returned, instead of just dir
    • this avoids redundantly re-computing the ports later.
  • avoid permission errors, e.g. trying to read .nrepl-port on macOS /Users/$USER/Desktop

And a minor CI improvement expressed in a separate commit.

I've QA'd all code paths - it works as intended:

Screen Shot 2023-08-10 at 16 29 05

Cheers - V

@vemv vemv requested a review from bbatsov August 10, 2023 14:36
@vemv
Copy link
Member Author

vemv commented Aug 10, 2023

@ag91 feel free to try this one locally.

curl https://patch-diff.githubusercontent.com/raw/clojure-emacs/cider/pull/3399.diff | git apply (over master) may be handy

(nrepl--port-from-file (expand-file-name ".nrepl-port" dir))
(nrepl--port-from-file (expand-file-name "target/repl-port" dir))
(nrepl--port-from-file (expand-file-name ".shadow-cljs/nrepl.port" dir))))
(condition-case nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking to tackle this with with-demoted-errors (although also ignore-errors would do here if you don't want to let the user know about what caused the ignored error): maybe it saves some code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Sounds reasonable.

Now that I think it, we may want to catch errors for each individual directory entry, instead of for the whole call, so that one bad dir doesn't cause all its neighbors to be excluded.

cider.el Outdated Show resolved Hide resolved
cider.el Outdated Show resolved Hide resolved
(setq paths (cons (match-string 1) paths)))))
(seq-uniq paths)))
Search for lein or java processes including nrepl.command nREPL"
(append (cider--invoke-running-nrepl-path #'cider--running-lein-nrepl-paths)
Copy link
Member

Choose a reason for hiding this comment

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

We don't expect duplicates here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't expect duplicates here?

They're handled in the cider--running-nrepl-paths callsite. Which seems better since cider-locate-running-nrepl-ports can cons an element to it, so you'd have to run seq-uniq again

@vemv vemv merged commit f376d2d into master Aug 11, 2023
20 checks passed
@vemv vemv deleted the 3390 branch August 11, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using cider-connect with localhost nrepls running, those ports are not shown among the available ones
3 participants