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

Better naming convention for Result type alias in errors module #15

Open
JadedEvan opened this issue Oct 27, 2020 · 3 comments
Open

Better naming convention for Result type alias in errors module #15

JadedEvan opened this issue Oct 27, 2020 · 3 comments

Comments

@JadedEvan
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

Would it be possible to have a better type alias name for the firestore_db_and_auth::errors::Result type? As a relative newcomer to Rust this was extremely unclear how to use this. Currently the module errors declares a type alias that is intended to capture a variety of different errors (see original source code)

// Current implementation
pub type Result<T> = std::result::Result<T, FirebaseError>;

It was very unclear to me that where I would call operations like document::read() that I had to use the module's Result and not std::result. This is not clearly laid out in the documentation anywhere. Based on the example below, I'm not sure that the intention of this change lead to better ergonomics

// Incorrect
let result: Result<MyFirestoreUser, firestore_db_and_auth::errors::FirebaseError> = documents::read(&session, "user", "123456");

// Correct
let result: firestore_db_and_auth::errors::Result<MyFirestoreUser> = documents::read(&session, "user", "123456");

Describe the solution you'd like

Perhaps just declare this alias as something more specific to this crate FirestoreResult?

I think this keeps the ergonomic efficiency and also makes it clear you're getting a different Rust type as a result of the operation.

let result: FirestoreResult<MyFirestoreUser> = documents::read(&session, "user", "123456");

Describe alternatives you've considered

Is there a way to import this type into lib/main.rs so that I don't have to properly scope every call to firestore_db_and_auth:errors::Result

Additional context

Running on v0.6.0

@davidgraeff
Copy link
Owner

Hi thanks for the feedback.

When I had designed the API this way of having an own Result type within an error module was kind of state of the art or let's say the most Rust idiomatic way. Error handling has changed since then in the community.

So I will give it a second thought here as well. Thanks again for the suggestions, maybe I'm going with these.

@JadedEvan
Copy link
Collaborator Author

If you're open to the suggestion I'm happy to take a pass at making the change. I think the more controversial decision would be about the new type name - FirestoreResult? Definitely understand that this was idiomatic Rust (and may still be) but I do think this would make it more intuitive and would make the code more easily understood. Let me know!

@davidgraeff
Copy link
Owner

I will add a FirestoreResult to the libraries directly/non-namespaced exported types as an alias for errors::Result.
(also recommended by https://doc.rust-lang.org/rust-by-example/error/result/result_alias.html)

It is still idiomatic Rust to have a Result type somewhere (see https://doc.rust-lang.org/rust-by-example/error/multiple_error_types/define_error_type.html#defining-an-error-type), so I will not remove the existing one.

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

No branches or pull requests

2 participants