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.

Sign up for GitHub

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

Jump to bottom

allow passing a module to methods #33403

Merged
merged 5 commits into from
Dec 11, 2019
Merged

allow passing a module to methods #33403

merged 5 commits into from
Dec 11, 2019

Conversation

rfourquet
Copy link
Member

I often wish to see methods implemented by a specific module. Of course I can do collect(Iterators.filter(x -> x.module == MyModule, methods(eltype))) (collect is quite necessary as otherwise all the methods are printed, because the iterator itself is printed).
But it's cumbersome and I'm not even sure method.module is documented.

This PR implements the API methods(f, [types], [module::Module]), a possible added convenience could be to also allow passing a list of modules.

@rfourquet rfourquet added the needs docs Documentation for this change is required label Sep 27, 2019
ararslan
ararslan reviewed Sep 29, 2019
View reviewed changes
base/reflection.jl Outdated Show resolved Hide resolved
@ararslan ararslan added needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Sep 29, 2019
@rfourquet
Copy link
Member Author

Bump! I need this all the time (I come back to this PR today because I wrote in the OP the incantation to a work-around).

@StefanKarpinski
Copy link
Member

If there's no review of this in the next couple of days, I'm just going to merge it.

vtjnash
vtjnash approved these changes Oct 28, 2019
View reviewed changes
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Yep, Jeff and I wanted this too recently. Seems like we can always change it later if we want it to behave differently (e.g. looking into submodules, etc.)

@rfourquet
Copy link
Member Author

Cool, I will add the tests and news. Actually, are you fine that instead of only m::Module, we allow m::Union{Module,AbstractArray{Module},Tuple{Module,...}) ? More often than not, I'm interested in passing 2 modules (which as a matter of fact are often nested modules...)

@vtjnash
Copy link
Member

vtjnash commented Oct 28, 2019

Perhaps it should be default m=() and check m isa Module ? _ === m : (isempty(m) || in(_, m)) in that case?

KristofferC
KristofferC reviewed Oct 28, 2019
View reviewed changes
base/reflection.jl Outdated
if isa(f, Core.Builtin)
throw(ArgumentError("argument is not a generic function"))
end
t = to_tuple_type(t)
world = typemax(UInt)
return MethodList(Method[m[3] for m in _methods(f, t, -1, world)], typeof(f).name.mt)
MethodList(Method[m[3] for m in _methods(f, t, -1, world) if mod === nothing || m[3].module == mod],
Copy link
Member

Choose a reason for hiding this comment

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

Could arguably be parentmodule(m[3].module).

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? If I want methods defined in a submodule S of P, then methods(f, S) will give nothing and methods(f, P) will give all methods of f whether they are in S or P...

Copy link
Member

@KristofferC KristofferC Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I think I meant m[3].module == mod || parentmodule(m[3].module) == mod.

Basically, if you are asking for the methods in Pkg you probably want the methods in Pkg.API as well? Anyway, doesn't have to be done now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes a "recursive" option would be useful, maybe as a keyword.

@rfourquet
Copy link
Member Author

Perhaps it should be default m=() and check m isa Module ? _ === m : (isempty(m) || in(_, m)) in that case?

Yes I had something like that in mind. I will add a commit.

@rfourquet rfourquet removed needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Oct 28, 2019
@rfourquet
Copy link
Member Author

Is there a bug with CI not being triggered? No indication of CI status here, and I see the same happening in other PRs too...

@ararslan
Copy link
Member

No indication of CI status here, and I see the same happening in other PRs too...

The status doesn't show up until the build starts, so if there's a backlog of builds, they may not show up for a bit. Looks like many of the checks have run now, but there are still more to go.

@StefanKarpinski
Copy link
Member

What about allowing passing a collection of modules and then having a function for iterating a module and all of its children? Then this would be done as methods(f, types, m) for just the module m or as methods(f, types, submodules(m)) for the recursive version.

@rfourquet
Copy link
Member Author

rfourquet commented Oct 30, 2019 •
edited
Loading

Yes that seems like a good idea, more orthogonal.

@rfourquet
Copy link
Member Author

Bump: would be great to have that in version 1.4!

rfourquet
rfourquet commented Nov 16, 2019
View reviewed changes
base/reflection.jl Outdated
"""
function methods(@nospecialize(f), @nospecialize(t))
function methods(@nospecialize(f), @nospecialize(t),
@nospecialize(mod::Union{Module,AbstractArray{Module},Tuple{Vararg{Module}}}=()))
Copy link
Member Author

Choose a reason for hiding this comment

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

I realize that by passing () explicitly, like in methods(f, (Int,), ()), the user would expect an empty result, whereas here we consider it means no filtering on the module... should we change the default to nothing? This use-case is very contrived, but it might happen programmatically... Hm, I think I will use nothing, it's more correct.

@rfourquet
Copy link
Member Author

Bump :)

@StefanKarpinski StefanKarpinski merged commit cb8e1cf into master Dec 11, 2019
@StefanKarpinski StefanKarpinski deleted the rf/method-module branch December 11, 2019 18:42
@JeffBezanson JeffBezanson mentioned this pull request Jan 6, 2020
Closed
@davidanthoff davidanthoff mentioned this pull request Mar 22, 2020
Closed
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
@rfourquet @KristofferC
allow passing a module to methods (#33403)
c034812 
* allow passing a module to methods

* add missing `=nothing` (ararslan)

Co-Authored-By: Alex Arslan <ararslan@comcast.net>

* allow specifying multiple modules

* add NEWS and compat annotations

* use `nothing` instead of `()` as a default value
@omus omus mentioned this pull request Oct 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ararslan ararslan ararslan left review comments

@KristofferC KristofferC KristofferC left review comments

@vtjnash vtjnash vtjnash approved these changes

@JeffBezanson JeffBezanson Awaiting requested review from JeffBezanson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rfourquet @StefanKarpinski @vtjnash @ararslan @KristofferC