clojure-emacs / cider

The Clojure Interactive Development Environment that Rocks for Emacs
https://cider.mx
GNU General Public License v3.0
3.54k stars 646 forks source link

Fix another error regex bug, limit check to JVM runtime #3698

Closed yuhan0 closed 4 months ago

yuhan0 commented 4 months ago

Fixes #3687

Not sure if the last commit is technically a breaking change.. the inconsistent var naming was bugging me, and these were introduced recently so it's unlikely that other packages depend on them.

Side note: eldev lint produces lots of linter errors unrelated to the changes here, it seems like the CI isn't flagging them so they've been accumulating from other PRs.


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

bbatsov commented 4 months ago

the inconsistent var naming was bugging me, and these were introduced recently so it's unlikely that other packages depend on them.

Yeah, that's fine. They were meant to be private, anyways, and it's extremely unlikely that someone decided to do something with them directly. The PR looks good at a glance, but I'll take a closer look tomorrow.

yuhan0 commented 4 months ago

For reference, I based the regexes directly off the format strings here: https://github.com/clojure/clojure/blob/clojure-1.10.0/src/clj/clojure/main.clj#L264

And made the judgement that errors thrown in :print-eval-resultand :read-eval-result phases (not covered previously) would be considered "runtime errors" by Cider.

yuhan0 commented 4 months ago

I take it that you verified that the Refactor error-matching regexes is covered?

Yup, although I didn't add a test for the :read-eval-result phase, (not even sure how one would trigger the error)

(cider-runtime-clojure-p) seems to consider clj and cljs as one

Aren't Clojuresicrpt repls a layer running inside a Clojure process? (throwing JVM errors upon macroexpansion etc.) I'm not a regular cljs user, not sure what's the appropriate thing to do there

vemv commented 4 months ago

Yup, although I didn't add a test for the :read-eval-result phase, (not even sure how one would trigger the error)

That would seem very much valuable -sounds like a question that you could get answered on #clojure

Aren't Clojuresicrpt repls a layer running inside a Clojure process? (throwing JVM errors upon macroexpansion etc.)

Easier and safer to check. I think it is queriable anyway whether the connection is a cljs one (in the same way that you can query the port out of a repl object).

If you need a modern project with cljs, I have https://github.com/reducecombine/icd.scroll at hand - it's my generic repro project with a very clear README.

yuhan0 commented 4 months ago

Just a heads up but I won't be able to devote much time over the next week on this, probably not to the extent of setting up and debugging an unfamiliar cljs env.

The way I see it, this PR is a regression fix for 3616 - I could separate out the more subjective commits if it helps with getting things merged.

bbatsov commented 4 months ago

I'm OK with the code in the current state and in general and given that "perfect" is the enemy of "done", I'll just go ahead and merge it. Still, it'd be nice to follow-up on this in the future and refine the error message handling for the various runtime environments.