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

Expose Cargo exit status if non-zero #13

Closed
ebkalderon opened this Issue Feb 11, 2016 · 5 comments

Comments

2 participants
@ebkalderon
Member

ebkalderon commented Feb 11, 2016

Right now, the Amethyst CLI client always returns zero when calling Cargo commands, regardless of whether Cargo itself returned errors. This is why the tests.sh file doesn't work as expected, returning "All tests pass!" even when that's obviously not true (see the comments on pull request #12 for an example).

This isn't too problematic since Amethyst and its toolchain are in the 0.X.Y release stage, but nonetheless this must be fixed before 1.0 rolls around.

@White-Oak

This comment has been minimized.

Show comment
Hide comment
@White-Oak

White-Oak Feb 11, 2016

Contributor

btw, I suppose cargo call should return Result, not Option. Right now option return in case of error is very confusing.

Contributor

White-Oak commented Feb 11, 2016

btw, I suppose cargo call should return Result, not Option. Right now option return in case of error is very confusing.

@ebkalderon

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Feb 11, 2016

Member

@White-Oak I chose Option because it looked a little less verbose to me. If I were to choose Result, I would have to make the return value be something like Result<(), &'static str>, which isn't particularly pretty. Edit: It appears this actually may be standard practice(?)

If a successful Cargo execution isn't meant to return anything, it's much easier to use Some(error) and None. However, it's a subjective issue of aesthetics, really, and if the use of Option is too confusing for people, I'll gladly switch it to Result.

Member

ebkalderon commented Feb 11, 2016

@White-Oak I chose Option because it looked a little less verbose to me. If I were to choose Result, I would have to make the return value be something like Result<(), &'static str>, which isn't particularly pretty. Edit: It appears this actually may be standard practice(?)

If a successful Cargo execution isn't meant to return anything, it's much easier to use Some(error) and None. However, it's a subjective issue of aesthetics, really, and if the use of Option is too confusing for people, I'll gladly switch it to Result.

@White-Oak

This comment has been minimized.

Show comment
Hide comment
@White-Oak

White-Oak Feb 11, 2016

Contributor

Ah, considering the issue.
Problem in #12 was not about not exposing Cargo fail status, but about tests.sh not checking exit status of amethyst new.
I've PRed the fix #14.

Contributor

White-Oak commented Feb 11, 2016

Ah, considering the issue.
Problem in #12 was not about not exposing Cargo fail status, but about tests.sh not checking exit status of amethyst new.
I've PRed the fix #14.

@ebkalderon

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Feb 12, 2016

Member

@White-Oak After thinking about it some more, I've come to agree with your stance on using Result over Option. However, this should probably be a separate pull request from #15, which implements the basic functionality. One thing at a time...

Edit: I'd recommend reducing the verbosity of Result a bit with a typedef:

pub type CargoStatus = Result<(), &'static str>;

That makes things like this look much, much cleaner, similar to Option. It's less confusing and more Rusty.

pub fn call(args: Vec<&str>) -> CargoStatus {
    // ...

    if output.task.success() {
        Ok(())
    } else {
        Err("Running cargo task failed")
    }
}
Member

ebkalderon commented Feb 12, 2016

@White-Oak After thinking about it some more, I've come to agree with your stance on using Result over Option. However, this should probably be a separate pull request from #15, which implements the basic functionality. One thing at a time...

Edit: I'd recommend reducing the verbosity of Result a bit with a typedef:

pub type CargoStatus = Result<(), &'static str>;

That makes things like this look much, much cleaner, similar to Option. It's less confusing and more Rusty.

pub fn call(args: Vec<&str>) -> CargoStatus {
    // ...

    if output.task.success() {
        Ok(())
    } else {
        Err("Running cargo task failed")
    }
}
@ebkalderon

This comment has been minimized.

Show comment
Hide comment
@ebkalderon

ebkalderon Feb 13, 2016

Member

Looks like with commit 3b416e0, this issue is now resolved.

Member

ebkalderon commented Feb 13, 2016

Looks like with commit 3b416e0, this issue is now resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment