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

Ensure ns-query map is built correctly #618

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

dpsutton
Copy link
Contributor

The required structure for the 'query/var' function is {:var-query {:ns-query {...}}. The older implementation did not grab those
keywords from the cider message and put them at the correct
level. they were left top level and then missed by the
query/namespace function.

In addition to this, there was a bug in orchard which had the wrong
prefix for inlined deps so these were not omitted in 2 places: both
in the client-side specification of which namespaces to ignore and
orchard's own mechanism to elide cider's internal namespaces.

See related:

Before submitting a PR make sure the following things have been done:

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings
  • You've updated the README (if adding/changing middleware)

Keep in mind that new cider-nrepl builds are automatically deployed to Clojars
once a PR is merged, but only if the CI build is green.

If you're just starting out to hack on cider-nrepl you might find
this article and the
"Design" section of the README extremely useful.

Thanks!

dpsutton pushed a commit to dpsutton/cider that referenced this pull request Jun 23, 2019
Confluence of bugs in orchard, cider-nrepl, and CIDER. CIDER was using
the wrong key to send excluded namespaces. cider-nrepl was not putting
this key in the right position for the orchard query. And orchard was
not correctly eliding cider.nrepl inlined deps.

- orchard: clojure-emacs/orchard#59
- nrepl: clojure-emacs/cider-nrepl#618
@dpsutton dpsutton force-pushed the apropos-var-query branch from 0e4e452 to ea6a468 Compare June 23, 2019 17:48
The required structure for the 'query/var' function is `{:var-query
{:ns-query {...}}`. The older implementation did not grab those
keywords from the cider message and put them at the correct
level. they were left top level and then missed by the
`query/namespace` function.

In addition to this, there was a bug in orchard which had the wrong
prefix for inlined deps so these were not omitted in _2_ places: both
in the client-side specification of which namespaces to ignore and
orchard's own mechanism to elide cider's internal namespaces.

See related:
- orchard clojure-emacs/orchard#59
- CIDER clojure-emacs/cider#2658
@dpsutton dpsutton force-pushed the apropos-var-query branch from ea6a468 to 99d6313 Compare June 23, 2019 17:51
dpsutton pushed a commit to dpsutton/cider that referenced this pull request Jun 23, 2019
Confluence of bugs in orchard, cider-nrepl, and CIDER. CIDER was using
the wrong key to send excluded namespaces. cider-nrepl was not putting
this key in the right position for the orchard query. And orchard was
not correctly eliding cider.nrepl inlined deps.

- orchard: clojure-emacs/orchard#59
- nrepl: clojure-emacs/cider-nrepl#618
bbatsov pushed a commit to clojure-emacs/cider that referenced this pull request Jun 24, 2019
Confluence of bugs in orchard, cider-nrepl, and CIDER. CIDER was using
the wrong key to send excluded namespaces. cider-nrepl was not putting
this key in the right position for the orchard query. And orchard was
not correctly eliding cider.nrepl inlined deps.

- orchard: clojure-emacs/orchard#59
- nrepl: clojure-emacs/cider-nrepl#618
(let [msg {:exclude-regexps ["^cider.nrepl" "^refactor-nrepl" "^nrepl"]
:query "spelling"}
query-map (#'apropos/msg->var-query-map msg)]
(is (contains? (:var-query query-map) :ns-query))
Copy link
Member

Choose a reason for hiding this comment

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

Won't it be better to just compare the result with the expected map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought about that but i think the returned map will have regex's rather than strings for the exclude-regexps and I wasn't sure of the semantics of comparing regexes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and it appears they compare by reference. (= #"bob" #"bob") is false. so it would require some wrangling that i wasn't into at the time :) I can take another crack at it later if you like.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. It's fine like this.

@bbatsov bbatsov merged commit 7b94bc7 into clojure-emacs:master Jun 25, 2019
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.

2 participants