Skip to content

Filter special form symbols from eldoc args list #166

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 1 commit into from
Mar 10, 2023
Merged

Filter special form symbols from eldoc args list #166

merged 1 commit into from
Mar 10, 2023

Conversation

quanticle
Copy link
Contributor

Fixes #165.

Currently, the eldoc args list for special forms includes the special form itself, which throws off the highlighting in Emacs' CIDER. This patch removes the special forms themselves from the args list. It specifically omits the Java interop special forms, because these, for some reason, do not include themselves in the :forms vector.

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

Thanks much for the accurate reports and initial PR!

@quanticle quanticle marked this pull request as draft January 26, 2023 06:24
@quanticle quanticle marked this pull request as ready for review January 27, 2023 10:48
@quanticle
Copy link
Contributor Author

quanticle commented Jan 27, 2023

I updated the PR to take the approach that you described. I've added an option for info*, :dedup-special-forms, which does the special form filtering for eldoc. I've broken out the filtering logic into a separate function, dedup-forms-list, which takes an info map, checks to see that it represents the metadata for a special form (by checking the :special-form flag), and then, filters out the special form itself from the :forms lists. I also added unit tests for dedup-forms-list and an additional unit test for info* that passes the :dedup-special-forms flag and verifies that info* returns the filtered metadata.

EDIT: The code to actually invoke info* with this flag will be in cider.nrepl.middleware.info/eldoc-reply. I'll have that PR up soon as well.

@bbatsov
Copy link
Member

bbatsov commented Jan 27, 2023

I'm a bit puzzled about the need to make this deduping conditional. Are there cases when we'd actually want this?

P.S. I'm also puzzled that for 10 years nobody reported this when in hindsight the problem is obvious. :D

@bbatsov
Copy link
Member

bbatsov commented Jan 27, 2023

I've also been thinking that if want to adjust this only for Eldoc we can just as easily do it on the Elisp side. The current approach will not solve the problem for someone running CIDER without cider-nrepl as nREPL's info op is different (simpler).

@quanticle
Copy link
Contributor Author

If a user wrote a function whose first parameter was the same name as the function itself, i.e:

(defn foo [foo bar baz]
  (println foo bar baz))

we probably would not want to filter out the first argument because it's the same name as the function itself.

I initially tried to do this in the elisp code, but the obstacle I ran into was the fact that eldoc-info in the elisp didn't have any information as to whether the symbol was a special form or not. As far as I understand, eldoc only expects function or variable, and cider-nrepl pretends that special forms are functions.

@quanticle
Copy link
Contributor Author

Actually, now that I take another look at it, I see that cider-eldoc-thing-type in cider-eldoc.el, does have code to handle special-form. However, in eldoc.clj, the code only seems to return one of variable or function.

So instead of filtering here, what I could do is update extract-eldoc in eldoc.clj to also indicate whether the value is a special form.

Then, on the elisp side, it seems like it would be more straightforward to implement the filtering logic.

@bbatsov
Copy link
Member

bbatsov commented Jan 27, 2023

I'd be fine with this, although this still leave the problem with nREPL without cider-nrepl (and orchard). This information will be missing there.

Give how few the special forms are and they've never changed I guess we can just hardcode them in the Elisp code as a quick solution.

@quanticle
Copy link
Contributor Author

I've written up the change on the CIDER side, in PR #3233.

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

lgtm!

Happy to see a simplified version - the less code the better!

Instead of treating special-forms and macros as functions, we should
send them as special-forms and macros, respectively, so that
CIDER (and other clients) can properly handle the arglists.
@bbatsov
Copy link
Member

bbatsov commented Feb 27, 2023

@quanticle Don't forget to add a changelog entry for this.

@bbatsov
Copy link
Member

bbatsov commented Mar 6, 2023

@quanticle Ping :-)

@bbatsov bbatsov merged commit 65e0b4d into clojure-emacs:master Mar 10, 2023
vemv pushed a commit to clojure-emacs/cider that referenced this pull request Jun 23, 2023
This fix is the CIDER companion to [this
PR](clojure-emacs/orchard#166) in orchard,
which lets us know if the symbol we're querying eldocs for is a
special form or an ordinary function.

This changes uses that information to deduplicate the arglists for the
special form. In the process, the change adds a
`cider-eldoc-format-special-form` function, which serves as a home for
special form formatting.

This change also adds a unit test for cider-eldoc to ensure that it
calls the appropriate formatting methods.
bbatsov pushed a commit to clojure-emacs/cider that referenced this pull request Jun 24, 2023
This fix is the CIDER companion to [this
PR](clojure-emacs/orchard#166) in orchard,
which lets us know if the symbol we're querying eldocs for is a
special form or an ordinary function.

This changes uses that information to deduplicate the arglists for the
special form. In the process, the change adds a
`cider-eldoc-format-special-form` function, which serves as a home for
special form formatting.

This change also adds a unit test for cider-eldoc to ensure that it
calls the appropriate formatting methods.
r0man added a commit to r0man/cider-nrepl that referenced this pull request Jun 26, 2023
Orchard changed how arglists-str are formatted for special forms in
this PR:

clojure-emacs/orchard#166

It looks like this broke the info tests in cider-nrepl. Assuming the
new behaviour is the correct one, I fixed the tests accordingly.
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.

orchard.eldoc/extract-arglists does not handle many special forms correctly
3 participants