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

require and in-ns ns in separate forms for cider-repl-set-ns #2994

Merged
merged 4 commits into from
Mar 18, 2021

Conversation

tomdl89
Copy link
Contributor

@tomdl89 tomdl89 commented Mar 17, 2021

I think I had to create a new nrepl response handler in order to ensure it wouldn't cause two new prompts. Tested in two cljs environments, where it works fine. Feedback very welcome!

@dpsutton
Copy link
Contributor

That handler should have a docstring indicating its subtle behavior: to swallow the nil that comes back from requiring. I always go back and forth if the name should be what it does, ie cider-repl-value-ignoring-repl-handler instead of cider-repl-require-ns-handler and then the call site gives it its meaning. Oh, this code requires the namespace but ignores the return value (which is always nil?)

@tomdl89
Copy link
Contributor Author

tomdl89 commented Mar 17, 2021

fix for #2993

@bbatsov
Copy link
Member

bbatsov commented Mar 18, 2021

Do we really need an extra handler for this? Seems to me that we can just do a sync eval for the require and then trigger the async eval for the in-ns. This should result in code that's more straightforward IMO.

cider-repl.el Outdated
(format "(do (require '%s) (in-ns '%s))" ns ns)
(format "(in-ns '%s)" ns))
(when cider-repl-require-ns-on-set
(cider-nrepl-request:eval (format "(require '%s)" ns)
Copy link
Member

Choose a reason for hiding this comment

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

Here I'd just do a sync eval of the require and we should be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome! I had a feeling there would be a more concise way of doing this. thanks

cider-repl.el Outdated
(format "(do (require '%s) (in-ns '%s))" ns ns)
(format "(in-ns '%s)" ns))
(when cider-repl-require-ns-on-set
(cider-nrepl-sync-request:eval (format "(require '%s)" ns)))
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget the connection param here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and the ns param?

Copy link
Member

Choose a reason for hiding this comment

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

It's not needed in this case.

@bbatsov bbatsov merged commit 0449bf8 into clojure-emacs:master Mar 18, 2021
@bbatsov
Copy link
Member

bbatsov commented Mar 18, 2021

Thanks!

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