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

Added PkgError throw to Pkg.status() in entry.jl with simple test. #18293

Closed
wants to merge 1 commit into from

Conversation

ras9841
Copy link

@ras9841 ras9841 commented Aug 30, 2016

This is my solution to issue #18206. This is my first contribution, so I welcome any feedback.

@kshyatt kshyatt added the packages Package management and loading label Aug 30, 2016
@@ -125,6 +125,7 @@ function installed(pkg::AbstractString)
end

function status(io::IO; pkgname::AbstractString = "")
if ispath(pkgname) || throw(PkgError("package $pkgname does not exist")) end
Copy link
Member

@ararslan ararslan Aug 30, 2016

Choose a reason for hiding this comment

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

The best way to conditionally throw exceptions like this is a little simpler; just omit if and end. For example,

ispath(pkgname) || throw(PkgError("Package $pkgname does not exist"))

Alternatively (preferred if the condition or error message is too long for one line), you can do

if !ispath(pkgname)
    throw(PkgError("Package $pkgname does not exist"))
end

@ararslan
Copy link
Member

Hey @ras9841, thanks for contributing! Overall this is a nice first PR. However, it looks like the addition of the error caused a test failure. In particular, check out this portion of the Travis log. The test that's failing is this one:

@test sprint(io -> Pkg.status(io)) == "No packages installed\n"

I think that behavior is still desired, so you'll have to find a way to preserve that.

@ras9841 ras9841 closed this Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants