-
Notifications
You must be signed in to change notification settings - Fork 176
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
Make the track-state
middleware invokeable directly
#799
Conversation
I think the change about the namespace should be done separately. |
CHANGELOG.md
Outdated
@@ -2,6 +2,13 @@ | |||
|
|||
## master (unreleased) | |||
|
|||
### New features | |||
|
|||
* Make the `track-state` middleware invokeable directly, by requesting the new `"cider/state"` op. |
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.
I'd name this get-state
or even better - get-ns-state
, so it's clearer what it does. It wasn't that important when it couldn't be triggered, but now it should be some verb in line with most ops.
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.
I chose it for consistency with this key https://github.com/clojure-emacs/cider/blob/eedbab26684cb1ce01a940b5b0da9ff78aeb4eac/cider-repl.el#L238 , however I'd agree that the alternatives are clearer
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.
Yeah, I get this, but I don't think such a name would be appropriate for something one can invoke. The response will obviously stay the same.
@@ -1302,7 +1288,7 @@ Returns:: | |||
|
|||
|
|||
|
|||
=== `log-add-appender` | |||
=== `cider/log-add-appender` |
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.
I'm guessing you were doing some experiments with this.
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.
cider.nrepl.middleware.log
uses ns-qualified ops, so the doc benefits from this tweak
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.
I think the change about the namespace should be done separately.
The doc one?
CHANGELOG.md
Outdated
@@ -2,6 +2,13 @@ | |||
|
|||
## master (unreleased) | |||
|
|||
### New features | |||
|
|||
* Make the `track-state` middleware invokeable directly, by requesting the new `"cider/state"` op. |
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.
I chose it for consistency with this key https://github.com/clojure-emacs/cider/blob/eedbab26684cb1ce01a940b5b0da9ff78aeb4eac/cider-repl.el#L238 , however I'd agree that the alternatives are clearer
@@ -1302,7 +1288,7 @@ Returns:: | |||
|
|||
|
|||
|
|||
=== `log-add-appender` | |||
=== `cider/log-add-appender` |
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.
cider.nrepl.middleware.log
uses ns-qualified ops, so the doc benefits from this tweak
My bad - I didn't read the diff carefully and I thought you were also adding a solution for #710 when I saw the update to the docs. :-) |
Thanks! |
There's a good chance that this is necessary, given thheller/shadow-cljs#1138
Even if not, I found an independently good use case for it:
nano src/conduit/events.cljs
and hit save[:app] Build completed. (263 files, 2 compiled, 0 warnings, 0.18s)
over stderrnrepl-make-response-handler
call, grep for such a string, and conditionallynrepl-send-request '("op" "cider/state") ,,,
from there.I have fully tested out this solution, it works well for its two foreseen use cases (working around the shadow-cljs issue, and responding to external events).
Cheers - V