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

Make @__DIR__ return pwd() when being run without a file #21759

Merged
merged 2 commits into from
May 14, 2017
Merged

Make @__DIR__ return pwd() when being run without a file #21759

merged 2 commits into from
May 14, 2017

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented May 9, 2017

Fixes #21753

@ararslan ararslan added filesystem Underlying file system and functions that use it REPL Julia's REPL (Read Eval Print Loop) labels May 9, 2017
@Keno
Copy link
Member

Keno commented May 9, 2017

Wouldn't this also change what this means inside packages?

@ararslan
Copy link
Member Author

ararslan commented May 9, 2017

As currently written, yes, which was an oversight on my part. But I think that can be fixed easily.

Keno
Keno reviewed May 9, 2017
View reviewed changes
base/loading.jl Outdated
@@ -517,8 +517,13 @@ function source_path(default::Union{AbstractString,Void}="")
end

function source_dir()
isinteractive() && return pwd()
Copy link
Member

Choose a reason for hiding this comment

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

Leftover code?

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, thanks!

Keno
Keno reviewed May 9, 2017
View reviewed changes
test/loading.jl Outdated
@@ -14,6 +14,9 @@ include_string_test_func = include_string("include_string_test() = @__FILE__", t

@test isdir(@__DIR__)
@test @__DIR__() == dirname(@__FILE__)
let exename = `$(Base.julia_cmd()) --precompiled=yes --startup-file=no`
@test readchomp(`$exename -E "@__DIR__" -i`) == string('"', pwd(), '"')
end
Copy link
Member

Choose a reason for hiding this comment

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

Test that you don't get pwd() when running in a script? E.g. by doing cd("/tmp") do @__DIR__; end?

@ararslan ararslan force-pushed the aa/at-dir-pwd branch 2 times, most recently from a14769a to c31f4d0 Compare May 9, 2017 23:04
@ararslan
Copy link
Member Author

The Windows failure is perplexing to me:

__DIR__: "\"C:\\\\projects\\\\julia\\\\julia-b569b378b9\\\\share\\\\julia\\\\test\""
pwd: "\"C:\\projects\\julia\\julia-b569b378b9\\share\\julia\\test\""

The already escaped backslashes get escaped again, which is wrong. I'm not sure whether that's related to the changes here. Input from any Windows experts would be much appreciated.

vtjnash
vtjnash reviewed May 10, 2017
View reviewed changes
test/loading.jl Outdated
@@ -14,6 +14,12 @@ include_string_test_func = include_string("include_string_test() = @__FILE__", t

@test isdir(@__DIR__)
@test @__DIR__() == dirname(@__FILE__)
let exename = `$(Base.julia_cmd()) --precompiled=yes --startup-file=no`
wd = string('"', pwd(), '"')
@test readchomp(`$exename -E "@__DIR__" -i`) == wd
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

use println(@__DIR__), so that it doesn't get handed to show to escape all of the characters (or set wd to sprint(show, pwd()))

@ararslan
Copy link
Member Author

Cool, now the macOS and 64-bit Windows failures are unrelated, and the relevant tests are passing on all platforms.

@StefanKarpinski
Copy link
Sponsor Member

I'm feeling a little iffy about how patchy the behavior is with this change – I like that we return pwd now in the REPL, I'm saying that we should perhaps do so unconditionally and/or return the empty string unconditionally.

@ararslan
Copy link
Member Author

You mean omit the interactivity check and just always return pwd() when there's no source file?

@StefanKarpinski
Copy link
Sponsor Member

Yes, although I guess the worry is that could be breaking. But I'm not sure what checking or nothing is useful for aside from discovering that you're in the REPL, in which case just calling isinteractive() seems like a better and more explicit way to discover that fact.

@ararslan ararslan changed the title Make @__DIR__ return pwd() when run interactively Make @__DIR__ return pwd() when being run without a file May 12, 2017
@ararslan ararslan added the breaking This change will break code label May 12, 2017
@ararslan
Copy link
Member Author

This should also go in NEWS, but I'll wait until CI finishes then put that in a CI skipped commit.

@ararslan ararslan added the needs news A NEWS entry is required for this change label May 12, 2017
@tkelman
Copy link
Contributor

tkelman commented May 12, 2017

is there no other case where source_path can return nothing?

@ararslan
Copy link
Member Author

Based on my understanding of the function's logic, I don't believe so.

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 12, 2017

@tkelman yes (specific cases include include_string, Core.include, and the -e flag), although this function is getting a rewrite too (#21746)

@ararslan ararslan merged commit 9b428dc into master May 14, 2017
@ararslan ararslan deleted the aa/at-dir-pwd branch May 14, 2017 06:24
@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@Keno Keno Keno left review comments

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

Assignees
No one assigned
Labels
breaking This change will break code filesystem Underlying file system and functions that use it REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ararslan @Keno @StefanKarpinski @tkelman @vtjnash @KristofferC