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

Regression in eldoc #133

Closed
vemv opened this issue Oct 1, 2021 · 8 comments · Fixed by #134
Closed

Regression in eldoc #133

vemv opened this issue Oct 1, 2021 · 8 comments · Fixed by #134

Comments

@vemv
Copy link
Member

vemv commented Oct 1, 2021

https://app.circleci.com/pipelines/github/clojure-emacs/cider-nrepl/448/workflows/ab0029aa-c7c9-45d2-9071-d5692ea11aad/jobs/3661

i.e. querying eldoc for non.existing/+ will show the doc for clojure.core/+.

This is caused by 7d7f752

It's not a critical bug, anyway @plexus could you please take a look?

@plexus
Copy link
Contributor

plexus commented Oct 1, 2021

How do you suggest addressing that, short of reverting that change?

@vemv
Copy link
Member Author

vemv commented Oct 1, 2021

Perhaps we can make the change opt-out via an option and make cider.nrepl.middleware.info opt out?

@bbatsov
Copy link
Member

bbatsov commented Oct 1, 2021

I don't think we need to overcomplicate this. There are already a ton of config options that nobody knows/uses.

i.e. querying eldoc for non.existing/+ will show the doc for clojure.core/+.

I though this was the intention of the change, so perhaps we should just update the tests in cider-nrepl to reflect this.

@vemv
Copy link
Member Author

vemv commented Oct 1, 2021

I don't think showing a wrong eldoc for a different ns is correct behavior.

i.e. if I'm editing a given ns, it's fair enough to assume that an unqualified + refers to clojure.core/+.

But if querying a different ns, other/+ it's definitionally something other than clojure.core/+. It cannot equal to clojure.core/+. (If it was identical, why would it be defined at all?)

@bbatsov
Copy link
Member

bbatsov commented Oct 1, 2021

But you can't know this before the other ns was actually evaluated, that's why I don't see much of a problem with this. You might get a false positive in an unevaluated ns, but you'd be fine otherwise. Am I missing something?

@vemv
Copy link
Member Author

vemv commented Oct 1, 2021

One cannot necessarily know if the ns in question is evaluated... with tools.namespace is less obvious than it sounds (e.g. it can unload namespaces, fail, leave things uneval'd)

I'll check out again the logic/integration in question. Maybe we can easily distinguish these two cases?

  • a ns name was not passed (infer OK)
  • a ns name was passed (infer NOK)

@plexus
Copy link
Contributor

plexus commented Oct 1, 2021

I think you're right, we can limit this fallback behavior to unqualified symbols.

@bbatsov
Copy link
Member

bbatsov commented Oct 1, 2021

Makes sense to me as well.

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 a pull request may close this issue.

3 participants