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

Expose errors to other crates to enable error chaining. #31

Closed
wants to merge 1 commit into from

Conversation

mre
Copy link
Contributor

@mre mre commented Jan 26, 2017

In order to fully leverage the power of error chain it would be nice to make the error module public. This way, chaining errors is possible from other crates.
I need this functionality for better error handling inside a new tool that depends on duct.rs.

For more information, see rust-lang-deprecated/error-chain#120

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling d0254fc on mre:patch-1 into 44c9270 on oconnor663:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling d0254fc on mre:patch-1 into 44c9270 on oconnor663:master.

@oconnor663
Copy link
Owner

Commented on rust-lang-deprecated/error-chain#120. I think the issue there is that the errors module is only in master.

@@ -1280,7 +1280,7 @@ impl ExpressionStatus {
}
}

mod errors {
pub mod errors {
Copy link
Owner

Choose a reason for hiding this comment

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

We have pub use errors::* on line 1311. Do we need both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually you're right. I made a mistake when importing duct::errors::Error into my own scope. The only thing missing then is releasing a new version of duct.rs. 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Err, no. It's required. 😁

pub mod is for exposing a mod to external crates.
pub use is to re-export a function inside another module. This allows you to present an external interface that may not directly map to your internal code organization.
(From the book).

To make sure, I've just tested it again.

Doesn't work with the master in my Cargo.toml:

duct = { git = "https://github.com/oconnor663/duct.rs.git" }

Error:

error: module `errors` is private
 --> src/errors.rs:6:20
  |
6 |         ShellError(duct::errors::Error, duct::errors::ErrorKind);
  |                    ^^^^^^^^^^^^^^^^^^^

error: module `errors` is private
 --> src/errors.rs:6:20
  |
6 |         ShellError(duct::errors::Error, duct::errors::ErrorKind);
  |                    ^^^^^^^^^^^^^^^^^^^

error: module `errors` is private
 --> src/errors.rs:6:41
  |
6 |         ShellError(duct::errors::Error, duct::errors::ErrorKind);
  |                                         ^^^^^^^^^^^^^^^^^^^^^^^

error: module `errors` is private
 --> src/errors.rs:6:41
  |
6 |         ShellError(duct::errors::Error, duct::errors::ErrorKind);
  |                                         ^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 4 previous errors

Works with the PR branch in my Cargo.toml:

duct = { git = "https://github.com/mre/duct.rs.git", branch = "patch-1" }

So, I guess we should still merge this. We could replace pub use errors::*; with use errors::*; though, as it's imported in the crate root.

Copy link
Owner

Choose a reason for hiding this comment

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

Any chance it would work if you replaced duct::errors::ErrorKind with duct::ErrorKind and so on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. You're right. I feel stupid now. 😞
Sorry for all the noise. Releasing the package is enough. 👍

Copy link
Owner

Choose a reason for hiding this comment

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

No worries! Believe it or not you're the first duct user that I've heard from, and I'm thrilled.

@oconnor663
Copy link
Owner

I'll make a new release as soon as CI finishes. It looks like the Windows build was broken, and somehow my AppVeyor badge got pointed to the wrong URL. Should be ok now :p

@oconnor663 oconnor663 closed this Jan 26, 2017
@oconnor663
Copy link
Owner

Just published 0.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants