add pathof(::Module) #28310

stevengj · 2018-07-27T17:53:39Z

Closes #27592. abspath(FooModule) returns the path of FooModule.jl. The package directory can be obtained from abspath(FooModule, "..", "..").

fredrikekre · 2018-07-27T17:55:55Z

Should this really be a Pkg function? Isn't this useful for other modules as well? Maybe dirname(::Module)?

stevengj · 2018-07-27T17:59:07Z

@fredrikekre, it doesn't work for arbitrary modules, since it only makes sense for modules that come from a standard Pkg directory structure. (Modules don't even have to come from files!)

KristofferC · 2018-07-27T18:32:00Z

True, but the answer to the question from a package will be loaded has nothing to do with the implementation of the package manager. Forcing a dependency on Pkg for something that it just directly asks Base code loading about seems unfortunate?

stevengj · 2018-07-27T18:36:36Z

Sure, it's easy to move this to base. What should the function be called, and should it be exported?

stevengj · 2018-07-27T18:37:43Z

How about abspath(::Module)?

ararslan · 2018-07-27T19:09:34Z

That feels like a little bit of a pun but I kind of like it.

fredrikekre · 2018-07-27T19:16:01Z

How about abspath(::Module)?

I would expect this to return /path/to/package/src/Package.jl and not /path/to/package/. Maybe returning /path/to/package/src/Package.jl is more useful anyway?

ararslan · 2018-07-27T19:28:26Z

Maybe returning /path/to/package/src/Package.jl is more useful anyway?

Yes I believe so, especially because AFAIK there's nothing that says a package has to have a particular directory structure, so it may be the case for example that a module is defined in /path/to/package/Package.jl with no src directory. So for full generality, it seems like the full path to the file in which the module was defined would be best. That's my $0.02. 🙂

StefanKarpinski · 2018-07-27T21:04:10Z

Maybe every module should just know what source file it was defined in with nothing being the answer if it was defined in the REPL or some other non-source-file location?

KristofferC · 2018-07-27T21:04:13Z

Yes I believe so, especially because AFAIK there's nothing that says a package has to have a particular directory structure,

Yes, there is, look for "src" in loading.jl.

iamed2 · 2018-07-27T21:07:27Z

Maybe every module should just know what source file it was defined in with nothing being the answer if it was defined in the REPL or some other non-source-file location?

Implicit const __FILE__ = @__FILE__ in each module?

ararslan · 2018-07-27T21:53:36Z

Yes, there is, look for "src" in loading.jl.

Oh. 😐 Alright then. Well, I still think returning the file is useful. 😛

stevengj · 2018-07-27T22:03:06Z

Changed to abspath and made to return the file.

stevengj · 2018-07-29T13:59:09Z

CI is looking good.

stevengj · 2018-07-29T19:48:44Z

Marking as triage as otherwise there is no non-deprecated way of getting this info without mucking around in Base internals.

JeffBezanson · 2018-07-30T16:01:29Z

@StefanKarpinski and/or @KristofferC will have to decide this one.

StefanKarpinski · 2018-07-30T16:06:55Z

I'm a little uneasy about overloading abspath for this but I suppose it seems ok. I'm also not super into the multiargument version since this seems like a somewhat more useful definition:

abspath(m, paths...) = joinpath(dirname(abspath(m)), paths...)

stevengj · 2018-07-30T16:17:34Z

@StefanKarpinski, I removed the paths... args.

StefanKarpinski · 2018-07-30T16:21:43Z

stdlib/Pkg/src/API.jl

@@ -430,7 +430,7 @@ function clone(url::String, name::String = "")
 end

 function dir(pkg::String, paths::AbstractString...)
-    @warn "`Pkg.dir(pkgname, paths...)` is deprecated; instead, do `import $pkg; abspath($pkg, \"..\", \"..\", paths...)`." maxlog=1
+    @warn "`Pkg.dir(pkgname, paths...)` is deprecated; instead, do `import $pkg; joinpath(dirname(abspath($pkg)), \"..\", paths...)`." maxlog=1


With the dirname call you don't want the .. anymore.

You still need .., because dirname will return the src directory, while Pkg.dir returns the parent directory.

Ah, gotcha.

ararslan · 2018-07-30T17:21:36Z

I'm a little uneasy about overloading abspath for this but I suppose it seems ok

It does feel like a bit of a pun. Packages are loaded based on LOAD_PATH; perhaps to get the path from which a package is loaded, one should use loadpath(::Module)?

stevengj · 2018-07-30T17:42:42Z

which(module)?

StefanKarpinski · 2018-07-30T17:55:54Z

Maybe where(m) instead?

ararslan · 2018-07-30T18:02:38Z

I don't particularly like where, both because we already use that as a (non-reserved) keyword and I don't find it very descriptive.

ararslan · 2018-07-30T18:03:23Z

Having path somewhere in the name of the function seems worthwhile, as it signifies that what you're asking for and getting is a path.

stevengj · 2018-07-30T18:22:37Z

My problem with loadpath is that it sounds like a verb: "load the path". I dunno, maybe pathof? But I don't have strong feelings about the spelling here and am happy to change the PR to whatever people want.

stevengj · 2018-07-30T18:28:01Z

A nice thing about pathof is that I can imagine it being extended to other things, e.g. file descriptors, IOStreams, Cmds, etcetera. (But we could also use abspath for this.)

ararslan · 2018-07-30T18:31:43Z

I think pathto reads a little nicer than pathof, since in prose you'd say "the path to X," but I'm also fine with abspath.

StefanKarpinski · 2018-07-30T19:04:32Z

I don't like pathto because this is not the path to a module—whatever that is; it's the path of the file in which the module was defined, which pathof seems to pithily describe. Moreover, it's reminiscent of nameof which is a similar kind of reflection function.

JeffBezanson · 2018-07-30T19:14:44Z

Isn't this a new feature, and thus can be designed after 1.0?

stevengj · 2018-07-30T19:18:47Z

@JeffBezanson, it is and it isn't — the problem is that an old feature, Pkg.dir, was deprecated in 0.7 and there is no documented replacement. So if we wait until 1.1 for this we will end up with a bunch of packages relying on undocumented Base internals.

ararslan · 2018-07-30T19:18:50Z

Yes, but it makes things like documentation deployment and coverage submission much easier in CI, since currently scripts have to call Pkg.dir. With this they would be able to do julia -e 'using Package; include(joinpath(pathof(Package), ...); dostuff'.

edit: 3 second ninja'd by Steven.

stevengj · 2018-07-30T19:19:33Z

Changed to pathof.

fredrikekre · 2018-07-30T19:31:34Z

Yes, but it makes things like documentation deployment and coverage submission much easier in CI, since currently scripts have to call Pkg.dir. With this they would be able to do julia -e 'using Package; include(joinpath(pathof(Package), ...); dostuff'.

For this particular case you shouldn't need Pkg.dir-like functionality though, see e.g. https://github.com/JuliaDiff/ForwardDiff.jl/blob/d7ebc3d2eca3c1c4b10815cff8900bee9a600761/.travis.yml#L18

ararslan · 2018-07-31T15:49:34Z

CI is green and we're waiting on a couple of other things before tagging RC1, and with any luck that will be tagged before this week's triage. So can we make a decision here in short order?

ararslan · 2018-07-31T16:22:57Z

Well that's a decision if I've ever seen one 😛

build LCM in release mode

add Pkg.dir(::Module)

a7e6f89

stevengj added the packages Package management and loading label Jul 27, 2018

stevengj requested a review from KristofferC July 27, 2018 17:53

use AbstractString, not String, in API

0fe4d43

stevengj added 2 commits July 27, 2018 17:59

rename to abspath(::Module)

12bfbbb

news

c02cdd9

stevengj changed the title ~~add Pkg.dir(::Module)~~ add abspath(::Module) Jul 27, 2018

depwarn causes test failure, just use warn

c50291f

stevengj added the triage This should be discussed on a triage call label Jul 29, 2018

ararslan mentioned this pull request Jul 30, 2018

Set VERSION to 0.7.0-rc1 #28294

Merged

13 tasks

remove paths... args

1dfb3da

StefanKarpinski reviewed Jul 30, 2018

View reviewed changes

handle Base.locate_package(pkgid) === nothing

7ae9fe3

abspath -> pathof

8ae2a18

stevengj added 2 commits July 30, 2018 15:20

fix NEWS

db35360

Base.locate_package already calls normpath

af08b64

stevengj changed the title ~~add abspath(::Module)~~ add pathof(::Module) Jul 30, 2018

StefanKarpinski merged commit e6c789a into master Jul 31, 2018

StefanKarpinski deleted the sgj/pkgdir branch July 31, 2018 15:51

quinnj referenced this pull request in JuliaRobotics/LCMCore.jl Aug 2, 2018

Merge pull request #38 from JuliaRobotics/build-release

a72f203

build LCM in release mode

Datseris mentioned this pull request Aug 5, 2018

updates for julia 0.7 JuliaCI/PkgBenchmark.jl#68

Merged

JeffBezanson removed the triage This should be discussed on a triage call label Aug 16, 2018

tpapp mentioned this pull request Oct 14, 2018

Extend edit for modules. #29636

Merged

KristofferC pushed a commit that referenced this pull request Feb 11, 2019

add pathof(::Module) (#28310)

ae9b717

add pathof(::Module) #28310

add pathof(::Module) #28310

Conversation

stevengj commented Jul 27, 2018 • edited Loading

fredrikekre commented Jul 27, 2018 • edited Loading

stevengj commented Jul 27, 2018

KristofferC commented Jul 27, 2018

stevengj commented Jul 27, 2018

stevengj commented Jul 27, 2018 • edited Loading

ararslan commented Jul 27, 2018

fredrikekre commented Jul 27, 2018

ararslan commented Jul 27, 2018

StefanKarpinski commented Jul 27, 2018

KristofferC commented Jul 27, 2018

iamed2 commented Jul 27, 2018

ararslan commented Jul 27, 2018

stevengj commented Jul 27, 2018

stevengj commented Jul 29, 2018

stevengj commented Jul 29, 2018

JeffBezanson commented Jul 30, 2018

StefanKarpinski commented Jul 30, 2018 • edited Loading

stevengj commented Jul 30, 2018

StefanKarpinski Jul 30, 2018

Choose a reason for hiding this comment

stevengj Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

StefanKarpinski Jul 30, 2018

Choose a reason for hiding this comment

ararslan commented Jul 30, 2018

stevengj commented Jul 30, 2018

StefanKarpinski commented Jul 30, 2018

ararslan commented Jul 30, 2018

ararslan commented Jul 30, 2018

stevengj commented Jul 30, 2018 • edited Loading

stevengj commented Jul 30, 2018 • edited Loading

ararslan commented Jul 30, 2018

StefanKarpinski commented Jul 30, 2018

JeffBezanson commented Jul 30, 2018

stevengj commented Jul 30, 2018

ararslan commented Jul 30, 2018 • edited Loading

stevengj commented Jul 30, 2018

fredrikekre commented Jul 30, 2018

ararslan commented Jul 31, 2018

ararslan commented Jul 31, 2018

stevengj commented Jul 27, 2018 •

edited

Loading

fredrikekre commented Jul 27, 2018 •

edited

Loading

stevengj commented Jul 27, 2018 •

edited

Loading

StefanKarpinski commented Jul 30, 2018 •

edited

Loading

stevengj Jul 30, 2018 •

edited

Loading

stevengj commented Jul 30, 2018 •

edited

Loading

stevengj commented Jul 30, 2018 •

edited

Loading

ararslan commented Jul 30, 2018 •

edited

Loading