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

Cider's namespaces and vars filtered from ns-list and apropos #347

Merged
merged 2 commits into from
Apr 30, 2016
Merged

Cider's namespaces and vars filtered from ns-list and apropos #347

merged 2 commits into from
Apr 30, 2016

Conversation

ckoparkar
Copy link
Contributor

@ckoparkar ckoparkar commented Apr 25, 2016

For clojure-emacs/cider#1564. I still have to write the tests, update documentation etc. But is the basic idea correct ?

And on emacs, we can have a custom var like,

(defcustom cider-filter-prefixes '("cider.nrepl" "refactor-nrepl")
  "Cider middleware would filter out information from namespaces starting with `cider-filter-prefixes'"
  :type 'list
  :group 'cider
  :package-version '(cider . "0.13.0"))

which will be nil, if we want all the internals visible. Also, if we jack-in from cider-nrepl project, we don't have to set it to nil. It picks up the un-inlined versions of deps. But its good to keep it configurable.

Some screenshots;

ns-browser
screen shot 2016-04-26 at 1 12 56 am

apropos
screen shot 2016-04-26 at 1 13 14 am

If the user types out the entire ns form the ns-browser, he'll still get the vars listed. But thats ok I guess.

@@ -39,24 +39,22 @@
(filter #(.startsWith (str %) project-root))
(mapcat ns-find/find-namespaces-in-dir))))

(defn inlined-dependency?
Copy link
Member

Choose a reason for hiding this comment

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

I would still have the inlided-deps filtered separately for clarity's sake.

@bbatsov
Copy link
Member

bbatsov commented Apr 26, 2016

In general the approach sounds reasonable. Maybe it'd be better to use filter regular expressions instead, so people would have more flexibility. Even this is better than the current situation, though.

@ckoparkar
Copy link
Contributor Author

@bbatsov Ok. I'll make all the changes today and finalise this PR with tests and stuff. You mean accept regexp from the emacs custom var ?

@bbatsov
Copy link
Member

bbatsov commented Apr 27, 2016

@bbatsov Ok. I'll make all the changes today and finalise this PR with tests and stuff. You mean accept regexp from the emacs custom var ?

A list of regexps.

@bbatsov
Copy link
Member

bbatsov commented Apr 27, 2016

@Malabarba what do you think about the config?

@Malabarba
Copy link
Member

I agree with using a regexp, though I have no opinion on whether it's better to have a list or a single one (they're pretty interchangeable).
One thing to keep in mind is that this will be a Clojure regexp, so

  1. it won't get any special support in Customize buffers,
  2. users will need to know they're supposed to write a Clojure regexp with twice the number of backslashes.

@arrdem
Copy link
Contributor

arrdem commented Apr 27, 2016

Is that really a detail you have to ah inflict upon the users? Is it unreasonable to re-compile on the Clojure side?

@Malabarba
Copy link
Member

@arrdem
If someone is willing to write a translator, that's fine by me. :-) Won't be an easy task, though.

Still, I'm pretty sure the regexp engines are not identical in terms of features, so we'd still need to tell users some detail about what they can't use.

@bbatsov
Copy link
Member

bbatsov commented Apr 27, 2016

I have no opinion on whether it's better to have a list or a single one (they're pretty interchangeable).

Seems to me a few regexps are more readable than one huge regexp.

One thing to keep in mind is that this will be a Clojure regexp, so

it won't get any special support in Customize buffers,
users will need to know they're supposed to write a Clojure regexp with twice the number of backslashes.

True. I don't think that's a big deal provided it's explained in the docstrings.

@ckoparkar
Copy link
Contributor Author

Yeah, I agree. A list would be easier to understand. I've tested this with regexps like ".*nrepl", "cider\.nrepl", "refactor-nrepl"

@@ -52,11 +52,22 @@
;; rewritten by dolly
(.startsWith ns-name "eastwood.copieddeps"))))

(defn internal-namespace?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a regex matcher on a list now. But I think this name makes it easier to read functions where this is used.

cskksc added 2 commits April 27, 2016 21:47
The ns-list and apropos op's take an optional `filter-regexps` param. All namespaces matching any regexp from this list is dropped from the result. The apropos op drops vars contained in such namespaces.
;; If we don't pass the second arg, some cider ns will be returned
(is (some #(re-find #".*nrepl" %) (n/loaded-namespaces)))
;; Shouldn't return any cider.nrepl namespaces
(is (not-any? #(re-find #".*nrepl" %)
Copy link
Contributor Author

@ckoparkar ckoparkar Apr 27, 2016

Choose a reason for hiding this comment

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

@Malabarba Changed it :-) . I got an email about your comment. But cannot actually see it on GH. Weird.

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 think the comment was on a commit. The rebase must have cleared everything.

@bbatsov
Copy link
Member

bbatsov commented Apr 29, 2016

Apart from the odd test failure, everything looks good to me here.

@Malabarba
Copy link
Member

Me too.

@ckoparkar
Copy link
Contributor Author

Great :-) This can be merged then ? I'll submit the cider PR tomorrow.

@bbatsov bbatsov merged commit e1ed08c into clojure-emacs:master Apr 30, 2016
@ckoparkar ckoparkar deleted the feature/hide-internals branch April 30, 2016 04:18
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.

5 participants