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

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

Merged
merged 6 commits into from
Jun 3, 2024

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Jun 2, 2024

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):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

cider-eval.el Outdated Show resolved Hide resolved
@bbatsov
Copy link
Member

bbatsov commented Jun 2, 2024

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.

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
@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 2, 2024

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.

Copy link
Member

@vemv vemv left a 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

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 2, 2024

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
Copy link
Member

vemv commented Jun 2, 2024

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.

@vemv vemv marked this pull request as draft June 3, 2024 00:20
@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 3, 2024

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 bbatsov marked this pull request as ready for review June 3, 2024 08:25
@bbatsov bbatsov merged commit 9f93a5f into clojure-emacs:master Jun 3, 2024
39 checks passed
@bbatsov
Copy link
Member

bbatsov commented Jun 3, 2024

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.

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.

Inline error overlays not displaying
3 participants