jmcnamara / rust_xlsxwriter

A Rust library for creating Excel XLSX files.
https://crates.io/crates/rust_xlsxwriter
Apache License 2.0
250 stars 23 forks source link

feature request: `Worksheet` and `SerializeFieldOptions` / `CustomSerializeField` methods should take the same format type #67

Closed lucatrv closed 6 months ago

lucatrv commented 6 months ago

Feature Request

Worksheet::*_format methods take a &Format, while SerializeFieldOptions::*_format and CustomSerializeField::*_format methods take a generic impl Into<Format>. @jmcnamara I report this for your reference, not because it is a bug, but because maybe you would like them to be aligned.

Just to make an example, the following code compiles:

static CENTER_FORMAT: LazyLock<Format> = LazyLock::new(|| {
    Format::new().set_align(rust_xlsxwriter::FormatAlign::Center)
});
worksheet.set_column_format(0, &CENTER_FORMAT)?;

while the following code does not compile:

static CENTER_FORMAT: LazyLock<Format> = LazyLock::new(|| {
    Format::new().set_align(rust_xlsxwriter::FormatAlign::Center)
});
let header_options = rust_xlsxwriter::SerializeFieldOptions::new()
        .set_header_format(&CENTER_FORMAT);

and should be written as:

static CENTER_FORMAT: LazyLock<Format> = LazyLock::new(|| {
    Format::new().set_align(rust_xlsxwriter::FormatAlign::Center)
});
let header_options = rust_xlsxwriter::SerializeFieldOptions::new()
        .set_header_format(&*CENTER_FORMAT);
jmcnamara commented 6 months ago

maybe you would like them to be aligned.

I would, but I would align them around format: impl Into<Format> and not format: &Format.

The From/Into allows the user to create a format from a number format string like this:

    let header_options = SerializeFieldOptions::new()
        .set_custom_headers(&[CustomSerializeField::new("cost").set_value_format("$0.00")]);

    // As opposed to:
    let value_format = Format::new().set_num_format("$0.00");

    let header_options = SerializeFieldOptions::new()
        .set_custom_headers(&[CustomSerializeField::new("cost").set_value_format(&value_format)]);

That is more useful for some of the secondary components that only require a number format Format. That isn't as useful for the worksheet() methods which is why (I think) I didn't do it initially.

lucatrv commented 6 months ago

Yes I agree

jmcnamara commented 6 months ago

Having thought about this a bit more and having done an initial implementation I have decided to leave is as is. That is:Worksheet will use format: &Format since that is a clearer expression of what the library requires and that secondary objects like Table and ConditionalFormat will use format: impl Into<Format> as a helpful syntactic sugar to convert strings to number formats, since that is often all the secondary objects need.

I may reconsider that in the future but for now I'm going to set it as "won't fix".

Thanks for highlighting it.