Skip to content

Move Result to top of core crate #825

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

Merged
merged 1 commit into from
Jun 17, 2022
Merged

Conversation

rylev
Copy link
Contributor

@rylev rylev commented Jun 17, 2022

This rather large PR is quite simple. It adds a pub use declaration to azure_core so that users can access the Result type alias from the root of azure_core like so: azure_core::Result. This is quite a common pattern in the Rust world.

The rest of the changes are simply moving all uses of azure_core's Result type to use this path so that we have consistency across the code base.

@rylev rylev requested a review from cataggar June 17, 2022 14:22
@rylev rylev force-pushed the top-level-result branch 2 times, most recently from bfdd8af to 2a1e82f Compare June 17, 2022 14:31
@rylev rylev force-pushed the top-level-result branch from 2a1e82f to c715917 Compare June 17, 2022 14:42
@bmc-msft
Copy link
Contributor

I was hoping to ask about doing the same.

@bmc-msft
Copy link
Contributor

This LGTM, the only thing I would have done differently is doing use azure_core::Result and then using Result, rather than repeatedly using azure_core::Result.

@@ -40,6 +40,7 @@ use uuid::Uuid;
pub use bytes_stream::*;
pub use constants::*;
pub use context::Context;
pub use error::Result;
Copy link
Member

Choose a reason for hiding this comment

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

Why just Result and not errror::*?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that though I think I'd leave that for a future PR.

@rylev
Copy link
Contributor Author

rylev commented Jun 17, 2022

This LGTM, the only thing I would have done differently is doing use azure_core::Result and then using Result, rather than repeatedly using azure_core::Result.

I purposefully went this route since this tends to be more idiomatic. Generally speaking, unqualified Result is reserved for std::result::Result while qualified Result is used for others. This is why in the std lib docs, you often see std::io::Result used as io::Result instead of just as Result.

This isn't a hard and fast rule though obviously.

@rylev rylev merged commit 1893763 into Azure:main Jun 17, 2022
@rylev rylev deleted the top-level-result branch June 17, 2022 16:12
@cataggar cataggar added the Azure.Core The azure_core crate label Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core The azure_core crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants