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

Simplify ZeroVec/VarZeroVec error handling, consolidate ULEError type #1389

Merged
merged 8 commits into from
Dec 14, 2021

Conversation

Manishearth
Copy link
Member

Prerequisite of #1079

Hitting a ZeroVec error likely means that you fed ZeroVec invalid data, and the utility of the error type is limited. Instead of having a complex heirarchy of error types that compose and can retain all error data from the source, we use a simpler ULEError type that broadly categorizes errors and includes the relevant type info.

This makes writing the proc macro (#1079) much easier since it does not need to generate error types as well. Furthermore, it simplifies ZeroVec for end users.

A change that I did not make here but perhaps should is renaming ULEError to ZeroVecError; since the place most users will see this is from ZeroVec and VarZeroVec. Thoughts?

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • utils/zerovec/src/varzerovec/borrowed.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@sffc
Copy link
Member

sffc commented Dec 13, 2021

A change that I did not make here but perhaps should is renaming ULEError to ZeroVecError; since the place most users will see this is from ZeroVec and VarZeroVec. Thoughts?

Or zerovec::Error ? (This is a utility crate so we can follow Rust conventions more than ICU4X which targets other programming languages)

Are there any other errors we surface in the zerovec crate?

sffc
sffc previously approved these changes Dec 13, 2021
utils/zerovec/src/ule/error.rs Outdated Show resolved Hide resolved
utils/zerovec/src/ule/error.rs Outdated Show resolved Hide resolved
utils/zerovec/src/ule/error.rs Outdated Show resolved Hide resolved
utils/zerovec/src/ule/error.rs Outdated Show resolved Hide resolved
.map_err(|_| VarZeroVecError::FormatError)?;
let len_bytes = slice.get(0..4).ok_or(ULEError::FormatError)?;
let len_ule =
PlainOldULE::<4>::parse_byte_slice(len_bytes).map_err(|_| ULEError::FormatError)?;
Copy link
Member

Choose a reason for hiding this comment

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

Thought: Should we add special parsing methods to PlainOldULE that are infallible?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already have From<[u8; N]>, but it's pretty moot here since it's tricky to statically prove that a subslice is of a given length (until someone writes a slice<N1, N2> function)

Co-authored-by: Shane F. Carr <shane@unicode.org>
@Manishearth
Copy link
Member Author

No, there are no other errors. My understanding of the convention for not prefixing errors is that it's kinda outdated: It was an aspirational convention at 1.0 but people never really took to it because it quickly gets confusing.

@Manishearth Manishearth requested a review from sffc December 14, 2021 03:58
@@ -7,42 +7,48 @@ use core::fmt;

/// A generic error type to be used for decoding slices of ULE types
#[derive(Copy, Clone, Debug)]
pub enum ULEError {
pub enum ZeroVecError {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: If you name this ZeroVecError, please move it up to the top level instead of nesting it under ule

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I continued reexporting it from ule for the convenience of glob imports

utils/zerovec/src/ule/error.rs Outdated Show resolved Hide resolved
@sffc
Copy link
Member

sffc commented Dec 14, 2021

No, there are no other errors. My understanding of the convention for not prefixing errors is that it's kinda outdated: It was an aspirational convention at 1.0 but people never really took to it because it quickly gets confusing.

I still find I prefer writing cratename::Error and having it "just work", such as when I write From impls for my own error type. I find that much easier than having to go to the docs and figure out what error name they used. I find that I very rarely need to use cratename::Error. But maybe application developers need that more than library developers.

@Manishearth
Copy link
Member Author

Manishearth commented Dec 14, 2021

Yeah in my experience app devs often need it more. It's a tricky tradeoff, but a core thing is that folks really dislike renaming imports (for whatever reason), and folks tend to avoid full paths for types.

@Manishearth Manishearth requested review from sffc and removed request for zbraniecki, gregtatum and nordzilla December 14, 2021 04:42
@Manishearth Manishearth merged commit 7ae1e6d into unicode-org:main Dec 14, 2021
@Manishearth Manishearth deleted the uleerror branch December 14, 2021 05:05
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.

2 participants