Deferred middleware extra fixes #444
Conversation
src/cider/nrepl.clj
Outdated
| [cider.nrepl.print-method])) | ||
|
|
||
| (def DELAYS (atom nil)) | ||
| (def DELAYS |
There was a problem hiding this comment.
Why not deferred-middleware? Seems like the best possible name to me.
There was a problem hiding this comment.
Those are not middleware as such, but handlers which are deferred and that. It's also a map of delay objects, thus DELAYS. delayed-handlers is probably a better name.
src/cider/nrepl.clj
Outdated
| "Map of `delay`s holding deferred middleware handlers." | ||
| (atom nil)) | ||
|
|
||
| (def LOCK |
There was a problem hiding this comment.
I really think this should be named differently - LOCK is way too generic (and as it's not a constant I don't think it should all-caps either).
There was a problem hiding this comment.
I use caps to emphasize that those a global variables. Some form of marking is needed to differentiate them from locals. All-caps is a useful marking technique and restricting it to only constants is a too massive of a restriction IMO.
I can make them dynamic and use *lock* if that's what you are more comfortable with. But we will never use the dynamic nature of the vars then.
As to the names, how about REQUIRE-LOCK?
src/cider/nrepl.clj
Outdated
| (delay | ||
| (require `~ns) | ||
| (resolve-or-fail `~fn-name))) | ||
| (locking LOCK |
There was a problem hiding this comment.
I wonder if we can't serialize this in some more elegant manner. This combination of delay + locking seems very unclojury to me.
There was a problem hiding this comment.
Not sure what is troubling you. Those are rarely used clojure functions, but they do precisely what's needed in that context. Anything else would likely imply more code. Do you have some concrete ideas?
- Add docs - `run-delayed-handler` --> `run-deferred-handler` - `handle` argument --> `trigger-it`
vspinu
force-pushed
the
deferred-middleware-extras
branch
from
7e4ebf6 to
75641b6
Compare
|
Ok, as per our discussion, I have improved further on docs and naming. |
| [cider.nrepl.print-method])) | ||
|
|
||
| (def DELAYS (atom nil)) | ||
| (def DELAYED-HANDLERS |
There was a problem hiding this comment.
ALL_CAPS is unidiomatic Clojure. Any reason we're doing that here?
There was a problem hiding this comment.
It's good to differentiate between local and global non-function vars. It enhances readability. Any form of marking down is fine with me, but ALL-CAPS seems nicest. Some use xyz_ for this purpose.
There was a problem hiding this comment.
Well, anything non-private is in effect global, so I'm fine with sticking with the standard notation. Anyways, that's something tiny.
These are extra fixes related to #438.