Skip to content

Fix cljs type not set correctly #3282

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

Merged
merged 3 commits into from
Dec 8, 2022

Conversation

benjamin-asdf
Copy link
Contributor

I introduced a bug with 24b9891
that made cljs repls not upgrade automatically anymore to cljs (repl-type).
What actually happened was that after the init function, it was set an
ultimate time, because of the "demunge" eval call I added.

  1. Move the connection capability block in fron of
    =cider-nrepl-init-function=, this would by itself already fix it
    because then we would eval as clj repl and in the end once as cljs
    repl (which sets the repl-type see cider-repl--state-handler)

  2. do not make the "demunge" eval call at all, when we already know
    the repl is going to be cljs (cljs or pending-cljs).
    (This would by itself also be a fix).

I introduced a bug with 24b9891
that made cljs repls not upgrade automatically anymore to cljs (repl-type).
What actually happened was that after the init function, it was set an
ultimate time, because of the "demunge" eval call I added.

1. Move the connection capability block in fron of
=cider-nrepl-init-function=, this would by itself already fix it
because then we would eval as clj repl and in the end once as cljs
repl (which sets the repl-type see cider-repl--state-handler)

2. do not make the "demunge" eval call at all, when we already know
the repl is going to be cljs (cljs or pending-cljs).
(This would by itself also be a fix).

(when cider-auto-mode
(cider-enable-on-existing-clojure-buffers))

(setf cider-connection-capabilities
Copy link
Member

Choose a reason for hiding this comment

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

I think for the sake of readability the capabilities setting should be moved to their own function.

(nrepl-sync-request:eval "cljs.core/demunge" (current-buffer) nil 'tooling)
"value")
(or
cljsp
Copy link
Member

Choose a reason for hiding this comment

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

I think for variables it's better to use a name like is-cljs as p stands for predicate, which by definition is a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah nice

@bbatsov
Copy link
Member

bbatsov commented Dec 8, 2022

Apart from my remarks the fix seems reasonable.

@bbatsov
Copy link
Member

bbatsov commented Dec 8, 2022

The lint CI check is failing because you've used tabs instead of spaces for indentation - please address this as well.

I introduced a bug with 24b9891
that made cljs repls not upgrade automatically anymore to cljs (repl-type).
What actually happened was that after the init function, it was set an
ultimate time, because of the "demunge" eval call I added.

1. Move the connection capability block in fron of
=cider-nrepl-init-function=, this would by itself already fix it
because then we would eval as clj repl and in the end once as cljs
repl (which sets the repl-type see cider-repl--state-handler)

2. do not make the "demunge" eval call at all, when we already know
the repl is going to be cljs (cljs or pending-cljs).
(This would by itself also be a fix).
@benjamin-asdf
Copy link
Contributor Author

wasn't sure if I should amend my commit, made a commit with the same message.

@bbatsov bbatsov merged commit cade685 into clojure-emacs:master Dec 8, 2022
@benjamin-asdf
Copy link
Contributor Author

@bbatsov damm I accidentally have a lisp error int the function (conn-buff, conn-buffer)
8aaf558

@ikappaki
Copy link
Contributor

ikappaki commented Dec 11, 2022

Hi @benjamin-asdf :)

I believe this change has caused a lint error, as per circle ci report:

https://app.circleci.com/pipelines/github/clojure-emacs/cider/1400/workflows/13ce40d2-a4c2-4a9d-a7e1-b0d48357d21b/jobs/6709

=> ci/circleci: test-lint — Your tests failed on CircleCI

eldev -dtT compile --warnings-as-errors
# ...
[00:00.623]  ELC      nrepl-client.el
[00:00.623]  Byte-compiling file `nrepl-client.el' early as `require'd from another file...
[00:00.623]  Loading file `nrepl-client.el' before byte-compiling it...
[00:00.736]  Loading the byte-compiled file `nrepl-client.elc'...

[00:00.799]  
             In end of data:
             cider-connection.el:322:81: Error: the function `cider-current-ns' is not
                 known to be defined.
[00:00.800]  Saving target dependencies to file `.eldev/28.2/target-dependencies.build'...

[00:00.800]  Failed to byte-compile `cider-connection.el'
[00:00.800]  Finished erroneously on Sun Dec 11 20:45:48 2022

@bbatsov
Copy link
Member

bbatsov commented Dec 12, 2022

@ikappaki Indeed. Some functions will need to be shuffled around if we really need to use cider-current-ns in this file, as cider-client depends on cider-connection and the function is actually defined in cider-client.

@benjamin-asdf
Copy link
Contributor Author

I have a new version of the function at a8b5c40 it doesn't need to user cider-current-ns turns out. We can pass nil, seems to be fine.

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.

3 participants