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

Error cleanup: standard pattern for I/O errors and enum naming #312

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

sffc
Copy link
Member

@sffc sffc commented Oct 9, 2020

I attempted three different patterns for I/O errors in three different modules. I settled on the one that uses .map_err(|e| (e, &path)) because it seems clean with minimal boilerplate and easy to port between modules.

@sffc sffc requested a review from zbraniecki October 9, 2020 02:36
@sffc sffc requested a review from a team as a code owner October 9, 2020 02:36
Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

I'm leaning toward the idea that most errors are "internal" and relevant only within the scope of a module inside a crate (say, plurals::operands::Error or plurals::parser::Error).
Usually, each crate will have one error that is "public" in a sense of - when a regular user uses our crate, they will have to write some handling of that error either by having functions that return it, or converting that error to their error.

In that case, I suggest to give it a unique name, so that users don't end up with:

use icu_datetime::Error as DateTimeError;
use icu_pluralrules::Error as PluralRulesError;
...

but I'm ok also just naming all errors Error and expecting it for now and then maybe we'll get to the point where we'll rename the public one later.

This looks good for me for now.

@filmil
Copy link
Contributor

filmil commented Oct 9, 2020

I'm leaning toward the idea that most errors are "internal" and relevant only within the scope of a module inside a crate (say, plurals::operands::Error or plurals::parser::Error).
Usually, each crate will have one error that is "public" in a sense of - when a regular user uses our crate, they will have to write some handling of that error either by having functions that return it, or converting that error to their error.

In that case, I suggest to give it a unique name, so that users don't end up with:

use icu_datetime::Error as DateTimeError;
use icu_pluralrules::Error as PluralRulesError;
...

but I'm ok also just naming all errors Error and expecting it for now and then maybe we'll get to the point where we'll rename the public one later.

This looks good for me for now.

@filmil filmil closed this Oct 9, 2020
@filmil filmil reopened this Oct 9, 2020
@filmil
Copy link
Contributor

filmil commented Oct 9, 2020

(disregard, wrong click)

@sffc
Copy link
Member Author

sffc commented Oct 9, 2020

Currently I use the name Error internally but rename it when exporting it in the lib.rs file.

@filmil
Copy link
Contributor

filmil commented Oct 9, 2020

Currently I use the name Error internally but rename it when exporting it in the lib.rs file.

FWIW, using renamed "long form" (DateTimeError) errors is consistent with our style guide.
It also (still) makes sense to me to use pithy names (like Error) for types that the user isn't going to interact with directly.

I think this is consistent with the observation that people prefer to type DateTimeError to datetime::Error, and that if we don't match that preference users are simply going to be renaming thing. So we meet the users where they are, and give them properly named types from the get-go.

@sffc sffc merged commit 8d3c8d5 into unicode-org:master Oct 9, 2020
@sffc sffc deleted the io-errors branch October 9, 2020 17:46
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.

3 participants