refactor: make DataTypeRef public and introduce a DataTypeTrait trait

[lukapeschke]

Hello there, I recently discovered DataTypeRef in the changelog, really nice addition!

However, it is not publicly available through the API, so I figured I wanted to make it public. I then realised that it would be convenient to have a DataType trait, in order to allow for generics. I was thinking about something like this:

use calamine::{CellType, DataTypeTrait, Range};

fn do_stuff_on_data_range<DT: CellType + DataTypeTrait + Debug>(data_range: &Range<DT>) {
    todo!()
}

Is this something you would be interested in ? If yes, we can probably find better naming for the trait and maybe a cleaner API ?

Cheers!

[lukapeschke]

@tafia this is an example of what this would allow us to do on the fastexcel side: https://github.com/ToucanToco/fastexcel/pull/147/files#diff-b918afe42fb2f27a4f1d2295adf56664cdf9f5f089202d0ddf9a48245e15a47bR42 🙂

[dimastbk]

Why is this (and similar methods) under the dates feature? This feature is needed only for converting excel date to chrono

[lukapeschke]

Because the as_{datetime,duration} are under the dates feature as well, so it was mainly for consistency.

[tafia]

Thanks for the PR.
I am not a fan of DataTypeTrait name. I'd rather have a name without Trait in it. Maybe a simpler Data (really opened to suggestions).

[lukapeschke]

I am not a fan of DataTypeTrait name. I'd rather have a name without Trait in it. Maybe a simpler Data (really opened to suggestions).

Maybe DataType as the trait name, and rename the DataType enum to DataTypeOwned ?

Apart from that, you wouldn't be opposed to having such a trait an making DataTypeRef public ?

[tafia]

No I am definitely not opposed to having such trait and exposing the Ref variant.

How about DataType for the trait, and Data/DataRef for the enums. The rationale is

• no Trait in name
• smaller names if used more frequently (I believe that Data will still be used the most)
• not a super fan of Owned variant.

An alternative? would be to use a Cow with the borrowed variant the owned one and the ref variant the Ref one.

[lukapeschke]

DataType and Data/DataRef sounds good 👍 I'll do the change asap

[tafia]

Great.
Can you please update the Changelog, as it is a breaking change, to be sure I won't forget to put it.
Thanks!

[lukapeschke]

@tafia I udapted the changelog in e2fe7f9 , do you want me to add something else ?

[tafia]

Awesome! Thanks!

[lukapeschke]

Thank you for the quick reviews!