OffsetResult should be a specialization of Result

[mzabaluev]

It's unfortunate that OffsetResult was copied from chrono's LocalResult, as that is non-idiomatic.

I think it should be more ergonomically defined as type OffsetResult<T> = Result<T, OffsetError<T>>;, with OffsetError taking the error cases. You can add utility methods to this specialization of Result with extension traits (which could be re-exported in a prelude mod).

[Yuri6037]

It's unfortunate that OffsetResult was copied from chrono's LocalResult, as that is non-idiomatic.

Why is it non-idiomatic? Could you explain a bit more? As for why I've chosen not to use a Result type is because I'm not sure if it's good to consider an ambiguous offset as an error. You seem to imply that ambiguous offsets is an error.

I think it should be more ergonomically defined as type OffsetResult<T> = Result<T, OffsetError<T>>;

I'm not sure that would be more ergonomic than the current definition. Using a standard Result type would be a breaking change and would be more restrictive with what facility is allowed to be implemented. Could you develop on what makes the current OffsetResult less ergonomic than a type alias to Result?

[mzabaluev]

Why is it non-idiomatic? Could you explain a bit more? As for why I've chosen not to use a Result type is because I'm not sure if it's good to consider an ambiguous offset as an error. You seem to imply that ambiguous offsets is an error.

OffsetResult::None is clearly an error case. It could be handled with the ? operator, which you cannot provide support for in stable Rust.

An ambiguous offset is arguably abnormal too: there is no way to proceed without deciding on which of the possible times to use. There are a lot of APIs which provide recovery information in the error type of Result that can be used to implement some sort of coping behavior, for example, std::str::Utf8Error.

Using a standard Result type would be a breaking change

Fair point. So this is more of a wishlist item for 3.0, then.

[Yuri6037]

Can you try the version on master (3.0.0-rc.3.0.0), I've made a replacement Result type alias with extensions as previously to minimize breakage.

[mzabaluev]

Can you try the version on master (3.0.0-rc.3.0.0), I've made a replacement Result type alias with extensions as previously to minimize breakage.

Looks good; I have provided some comments.

The None variant name does not feel right now that it's an error, perhaps Invalid or OutOfRange would be better?

[Yuri6037]

Looks good; I have provided some comments.

I've seen that, for one of them it's out of date as the auto format pass in the CI workflow fixed it. About the naming consistency issue, I have no idea how to fix it without having either inconsistent names related to what the function does or impossibility to call one of the functions due to name conflict.

The None variant name does not feel right now that it's an error, perhaps Invalid or OutOfRange would be better?

None means that there's no matching offset, it does not mean Invalid or even OutOfRange. Perhaps Undefined would be better, what do you think?

EDIT: I've published a new release candidate (5.0.0) which should improve APIs with a better name for map_all.

[mzabaluev]

Perhaps Undefined would be better, what do you think?

Yeah; *Error::None just rubs me the wrong way due to old trauma from working with COM 😅

with a better name for map_all.

I thought, too, that there should be a more domain-specific name for the extension method on Result, to avoid conflicts if map_all ever appears as a Result intrinsic method or in another extension trait. map_offsets maybe?

[Yuri6037]

Perhaps Undefined would be better, what do you think?

Yeah; *Error::None just rubs me the wrong way due to old trauma from working with COM 😅

with a better name for map_all.

I thought, too, that there should be a more domain-specific name for the extension method on Result, to avoid conflicts if map_all ever appears as a Result intrinsic method or in another extension trait. map_offsets maybe?

I'm not sure about map_offset because OffsetResult doesn't always store an offset. I came up with map_and_err to try to signify that the function maps both the Ok variant and the Err variant.

[mzabaluev]

OffsetResult doesn't always store an offset.

So it's like Option::map that converts None to None in the return type's domain without applying the map closure.

Result has the intrinsic map and map_err methods which map the Ok and Err payloads. Alongside these, I think it's not clear what map_and_err really does.

[Yuri6037]

OffsetResult doesn't always store an offset.

So it's like Option::map that converts None to None in the return type's domain without applying the map closure.

Result has the intrinsic map and map_err methods which map the Ok and Err payloads. Alongside these, I think it's not clear what map_and_err really does.

map_and_err just calls map on the Ok variant as well as on the OffsetError. In other words it's a shortcut to res.map(|a| f(&a)).map_err(|e| e.map(f)) with res being an OffsetResult, f the function which converts from one value type to another. The value types are usually some kind of timezone offset for the source result type and OffsetDateTime for the the destination type. What name do you think better matches this behavior?

EDIT: note that in some of my projects I may end up mapping OffsetDateTime into another a PrimitiveDateTime or even into another OffsetDateTime, which is why I say "usually" as it's not always the case.