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

Remove package extensions in Pkg functions #20681

Merged
merged 2 commits into from
Feb 27, 2017

Conversation

fredrikekre
Copy link
Member

@fredrikekre fredrikekre commented Feb 19, 2017

This implements #20630 and allows for eg.

Pkg.add("SpecialFunctions.jl")
Pkg.rm("SpecialFunctions.jl")
...

by stripping of the terminating .jl extension.

TODO:

  • add tests (ideas?)
  • documentation (where should this be documented?)

fix #20630

@ararslan ararslan added needs docs Documentation for this change is required needs tests Unit tests are required for this change domain:packages Package management and loading labels Feb 19, 2017
@ararslan
Copy link
Member

Thanks for working on this! As a test, you could add/remove the Example package both with and without the .jl suffix.

test/pkg.jl Outdated
Pkg.status("Example.jl", iob)
str = chomp(String(take!(iob)))
@test endswith(str, string(Pkg.installed("Example.jl")))
@test isempty(Pkg.dependents("Example.jl"))
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems a little fragile, anyone could break this by adding a package that depends on Example in the future (which is kinda silly, but allowed)

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 just copied from the usual test suite:

@test isempty(Pkg.dependents("Example"))

Copy link
Member Author

Choose a reason for hiding this comment

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

But yeah, both tests seem fragile.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Seems pretty hypothetical. If anyone does that, we can fix the tests then.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would immediately break the tests of all past versions of julia though, which isn't great. Would be better to move this to a section of the tests that's running against a known past commit of metadata, rather than the latest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to push a commit to this branch fixing this and the test linked in my comment above or handle it in a separate PR. I am not sure how I should go about it.

@fredrikekre fredrikekre changed the title [WIP/RFC] Remove package extensions in Pkg functions Remove package extensions in Pkg functions Feb 19, 2017
base/pkg/pkg.jl Outdated
@@ -37,6 +37,10 @@ const cd = Dir.cd

dir(path...) = Dir.path(path...)

# remove extension .jl
const PKGEXT = ".jl"
splitjl(pkg::AbstractString) = endswith(pkg, PKGEXT) ? splitext(pkg)[1] : pkg
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it really matters but pkg[1:(end - 3)] would be much faster...

@ararslan ararslan added domain:packages Package management and loading and removed needs tests Unit tests are required for this change domain:packages Package management and loading needs docs Documentation for this change is required labels Feb 20, 2017
@@ -171,6 +171,14 @@ git config --global url."https://".insteadOf git://

However, this change will be system-wide and thus the use of [`Pkg.setprotocol!()`](@ref) is preferable.

!!! note
The package manager functions accept package names with the `.jl` extension:
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably word this to say that it's possible but is stripped out internally so as not to encourage this style of use. Maybe "The package manager functions also accept the .jl suffix on package names, though the suffix is stripped out internally."

@fredrikekre
Copy link
Member Author

Failures seems unrelated? Feel free to restart.

@KristofferC
Copy link
Sponsor Member

to avoid fragility in the case that any future package
happens to depend on Example.jl
@fredrikekre
Copy link
Member Author

Bump. (Too late for this to be merged?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically remove .jl termination in Pkg.add
6 participants