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

Add pkgdir(m::Module) #33128

Merged
merged 9 commits into from
Oct 30, 2019
Merged

Add pkgdir(m::Module) #33128

merged 9 commits into from
Oct 30, 2019

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Aug 31, 2019

Edit: function now named pkgdir(m::Module) but was originally proposed as rootof(m::Module)


Provides a convenient way to get to the root directory (the parent directory of src) of the package that imported module m.

Closely equivalent to:

dirname(dirname(pathof(Base.moduleroot(m))))

Submodules resolve the same root:

import Foo
import Foo.SubFoo

rootof(Foo) == rootof(SubFoo)

One notable use case would be getting to package examples easily:

import Foo
examplesdir = joinpath(rootof(Foo),"examples")

Discussed in #31638

@IanButterworth
Copy link
Member Author

IanButterworth commented Aug 31, 2019

N.B. the submodule test required adding a submodule to test/project/deps/Foo1. I'm not sure whether that breaks any other tests
Edit: It doesn't seem to

base/loading.jl Outdated
rootmodule = Base.moduleroot(m)
path = pathof(rootmodule)
path === nothing && return nothing
return joinpath(splitpath(path)[1:end-2]...)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn’t the usual dirname(dirname(path)) be slightly more efficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

Your judgement is best here.. so I've switched it out

@StefanKarpinski
Copy link
Member

Looks good to me. I'm tagging for triage just to discuss the API.

@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Sep 5, 2019
@mschauer
Copy link
Contributor

mschauer commented Sep 6, 2019

A minor snag is the discrepancy in meaning of root between moduleroot and rootof(module). Can one derive a name from basedirectory?

@IanButterworth
Copy link
Member Author

IanButterworth commented Sep 6, 2019

That is unfortunate.

basedirectory is good but perhaps basedir given isdir and walkdir etc exist?

Also, not that I’m suggesting changing it, but rootmodule might have been a better name for moduleroot.

@StefanKarpinski
Copy link
Member

How about pkgdir(::Module)? After all, it is the replacement for Pkg.dir(::String) and it only returns a path if the module belongs to a package. The other option would be to define it in Pkg and then have it be Pkg.dir(::Module).

@IanButterworth
Copy link
Member Author

pkgdir(::Module) wins for simplicity given no need to Using Pkg. Especially if the main point here is competing with the simplicity of dirname(dirname(pathof(Module)))

But I like the mentality of this being a Pkg function too. Pkg.dir(::Module) makes a lot of sense.

How about both?

@StefanKarpinski
Copy link
Member

How about both?

Eh, I'd prefer not to have two gratuitously different ways to spell the same thing.

@IanButterworth
Copy link
Member Author

I’d go for the former then, given convenience

@vtjnash
Copy link
Member

vtjnash commented Sep 8, 2019

only returns a path if the module belongs to a package

Yeah, that's a pretty serious issue that almost makes these functions worse than useless. I've been intending to eventually get around to fixing that. I guess this is good motivation for me to address both at the same time. So, I'm good with merging this now, with the intention that the implementation will be completely redone at some point (so that it'll still typically give this answer, but actually give it correctly in all cases).

@IanButterworth
Copy link
Member Author

Just to check, I’d need to rename the function to pkgdir() before merge, if that’s what the consensus is?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Sep 8, 2019

I've been intending to eventually get around to fixing that.

How do you propose doing that? This function returns the root directory of the package that a module comes from. It can't, by definition, do that unless the module comes from a package.

So, I'm good with merging this now, with the intention that the implementation will be completely redone at some point (so that it'll still typically give this answer, but actually give it correctly in all cases).

Before you do any of that, please explain what you have in mind.

@KristofferC
Copy link
Member

Giving the path to the file where the module was defined (which is what pathof) does now seems good.

One notable use case would be getting to package examples easily:
examplesdir = joinpath(rootof(Foo),"examples")

If you just have the packages added then that will take you to some weird path where the examples are read only which is probably not where you want to be. And if you have the package deved, it is simple to cd there or copy paste the output from Pkg.status.

@IanButterworth
Copy link
Member Author

If you just have the packages added then that will take you to some weird path where the examples are read only which is probably not where you want to be. And if you have the package deved, it is simple to cd there or copy paste the output from Pkg.status.

I think there's a strong case for easy read-only access to dirs like examples, and that's the main purpose of examples. To be quick ways to showcase how to use the package, that can then be copied elsewhere and modified. Being able to just run them with minimal code seems helpful to me.

And dev-ing packages describes another kind of user. I don't think people would be looking to modify the examples generally. Copying the path out of Pkg.status is simple, but it's manual.

I just see a lot of dirname(dirname(pathof(m))) in example docs and it seems like it should be simpler.

@JeffBezanson
Copy link
Member

+1 to pkgdir instead of rootof; seems much clearer.

@IanButterworth IanButterworth changed the title Add rootof(m::Module) Add pkgdir(m::Module) Sep 12, 2019
@IanButterworth
Copy link
Member Author

I've switched out the name to pkgdir()

@fredrikekre
Copy link
Member

Do we need two functions that do (basically) the same thing?

@JeffBezanson
Copy link
Member

pathof could be extended to also work on e.g. methods and types.

@JeffBezanson
Copy link
Member

Triage is ok with this.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Sep 12, 2019
@IanButterworth
Copy link
Member Author

Could this get into v1.3, or is that window closed?

@fredrikekre
Copy link
Member

Feature freeze was August 15.

@IanButterworth
Copy link
Member Author

ok, thanks @fredrikekre

@StefanKarpinski
Copy link
Member

Do we need two functions that do (basically) the same thing?

I don't get this comment. @fredrikekre, what's the other function that's the same as this?

@IanButterworth
Copy link
Member Author

It'd be nice to get this into 1.4 🙏🏻

@StefanKarpinski StefanKarpinski merged commit a92f566 into JuliaLang:master Oct 30, 2019
@fredrikekre fredrikekre added the needs news A NEWS entry is required for this change label Nov 13, 2019
@fredrikekre
Copy link
Member

Looks like this is missing from NEWS.md

IanButterworth added a commit to IanButterworth/julia that referenced this pull request Feb 5, 2020
KristofferC pushed a commit that referenced this pull request Mar 4, 2020
* add note for new function pkgdir() (#33128)


Co-Authored-By: Fredrik Ekre <ekrefredrik@gmail.com>
Co-authored-by: Viral B. Shah <viral@juliacomputing.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
@IanButterworth IanButterworth deleted the rootof branch February 13, 2021 15:10
tlnagy added a commit to tlnagy/FluorescenceExclusion.jl that referenced this pull request Feb 11, 2022
testing fails because pkgdir wasn't added till 1.4: see
JuliaLang/julia#33128
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs news A NEWS entry is required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants