info and eldoc ops: accept a Compliment-style context parameter
#815
Conversation
| (and ns sym) (info/info* info-params) | ||
| (and class member) (info/info-java class member) | ||
| :else (throw (Exception. |
There was a problem hiding this comment.
The throw wasn't compatible with the new logic. nil is more flexible and will result in a graceful op-specific reply (e.g. status no-info or no-eldoc)
|
@alexander-yakushev - feel free to check out this PR. You may be happy to see Compliment used in a new way! Maybe later as things get more time-proven, we may want to have an explicit API just for this kind of purpose. |
| [{:keys [ns sym class member context] | ||
| legacy-sym :symbol | ||
| :as msg}] | ||
| (let [sym (or (not-empty legacy-sym) |
There was a problem hiding this comment.
Why do you need not-empty here?
There was a problem hiding this comment.
For discarding empty strings
I don't necessarily expect them, but I've found it's a good habit to handle them.
There was a problem hiding this comment.
Yeah, I figured as much although clients are generally expected to omit params instead of sending empty strings. I don't think we've got any such checks in cider-nrepl, that's why I've mostly done "truthy" check instead. Anyways, not a big deal right now, but something we can discuss later for the sake of consistency.
There was a problem hiding this comment.
🤝
In an ideal world we'd have something like Spec checking/coercion at the edges
| (let [sym (or (not-empty legacy-sym) | ||
| (not-empty sym)) | ||
| class (try | ||
| (or (when (and (seq class) |
There was a problem hiding this comment.
Is this nil-punning here? I still think it doesn't read very well. :D
There was a problem hiding this comment.
Not sure of what the alternative would be
(I had written a suggestion, but it wasn't accurate)
There was a problem hiding this comment.
I'm fine either way - I just don't like much mixing the use of seq and not-empty. Rich liked seq, but not-empty was other later on for those who felt seq wasn't very intention revealing.
There was a problem hiding this comment.
(my main point was the one I made earlier - that probably checking for empty strings is redundant, although I understand your reasoning as well)
There was a problem hiding this comment.
A nice convention is to exclusively use seq as a condition, and not-empty as an object one intends to further use
|
Other than the slightly confusing usage of |
vemv
force-pushed
the
812--compliment-inference
branch
from
785d21f to
94cd87a
Compare
vemv
force-pushed
the
812--compliment-inference
branch
from
485655a to
614df43
Compare
Fixes #812
Tested out locally - works as intended!
Cheers - V