tafia / calamine

A pure Rust Excel/OpenDocument SpreadSheets file reader: rust on metal sheets
MIT License
1.6k stars 155 forks source link

refactor: make `DataTypeRef` public and introduce a `DataTypeTrait` trait #390

Closed lukapeschke closed 5 months ago

lukapeschke commented 6 months ago

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 commented 6 months ago

@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 :slightly_smiling_face:

tafia commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

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

lukapeschke commented 5 months ago

DataType and Data/DataRef sounds good :+1: I'll do the change asap

tafia commented 5 months ago

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 commented 5 months ago

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

tafia commented 5 months ago

Awesome! Thanks!

lukapeschke commented 5 months ago

Thank you for the quick reviews!