-
-
Notifications
You must be signed in to change notification settings - Fork 643
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
Fix another error regex bug, limit check to JVM runtime #3698
Conversation
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. |
Merge with cider-clojure-unexpected-error into a single regex, Match :read-eval-result and :print-eval-result error phases
Mark the rx spec defconst vars as private Vars named `-regexp` should be strings, not rx specs
For reference, I based the regexes directly off the format strings here: And made the judgement that errors thrown in |
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.
LGTM in general, however:
- I take it that you verified that the
Refactor error-matching regexes
is covered? (cider-runtime-clojure-p)
seems to consider clj and cljs as one
Yup, although I didn't add a test for the
Aren't Clojuresicrpt repls a layer running inside a Clojure process? (throwing JVM errors upon macroexpansion etc.) |
That would seem very much valuable -sounds like a question that you could get answered on #clojure
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. |
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. |
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. |
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):
eldev test
)eldev lint
) which is based onelisp-lint
and includescheckdoc
, check-declare, packaging metadata, indentation, and trailing whitespace checks.