Add Hash trait on FixedOffset

[LuoZijun]

No description provided.

[quodlibetor]

This PR doesn't actually add a Hash impl, as far as I can tell.

What is Ord supposed to mean with timezones? They form a loop, you get back to the beginning by going past the end. Can you explain your use case?

[Enet4]

I guess that this is an attempt to conform with the C-COMMON-TRAITS guideline. This is so that users can rely on these traits (regardless of semantics, e.g. as BTreeMap key) without having to create newtypes.

But you're right, since the fixed offset timezone can be interpreted as having a cyclic ordering, deriving Ord might be confusing here, and should at least be very well documented to mean a (non-cyclic) total ordering.

[quodlibetor]

Thanks that's a good point. I would like to have Ord and if we can't have it we should document why it isn't there. I'd like to have a solid understanding of what the use-cases are -- as is, sticking a TZ into a BTreeMap would probably lead to surprising behavior?

If we're going to add it we should not derive it, we should do a separate impl so that we can add doc-comments about the actual behavior.

Also, I forgot to say: thanks for the PR @LuoZijun!

[LuoZijun]

@quodlibetor

I am using chrono to implement a built-in object Date of an ECMAScript language. This object looks like this:

#[derive(Debug, PartialEq, Eq, Copy, Clone)]
pub struct Datetime {
     Utc: NaiveDateTime,
     Offset: FixedOffset,
}

In the interpreter implementation, I need to store this structure in a HashMap (@Enet4 is right), so I need to implement the Hash and Eq and Ord features.

But currently our FixedOffset structure does not seem to implement these features automatically, so I need to implement it manually.

For some language reasons (I do not speak english), I'm sorry I didn't specify this on the PR

From Google Translate

[quodlibetor]

Thanks for the explanation! Google translate is fantastic.

For just a HashMap you shouldn't need Ord, the docs clearly state that just PartialEq + Eq + Hash is required. Ord is only required for a BTreeMap. Adding Hash is a trivial thing to do, and if you just add a test that verifies that we can insert a FixedOffset into a HashMap I'm happy to accept a PR that adds that derive.

If you still want Ord then I think we'd need a variety of tests that show what the behavior actually is so that we can discuss what including it means for users. If you still want this then I think you should probably split out the Ord into a separate PR so that we can unblock your usage of Hash more quickly.

[LuoZijun]

Updated.