jmcnamara / rust_xlsxwriter

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

Bug: issue with serde `skip_serializing_if` #71

Closed jmcnamara closed 9 months ago

jmcnamara commented 9 months ago

Current behavior

When using skip_serializing_if on a field the output can become unaligned.

Expected behavior

Skipping a value should maintain the correct incrementatal row number.

Sample code to reproduce

use rust_xlsxwriter::{ExcelSerialize, Format, Workbook, XlsxError};
use serde::Serialize;

fn main() -> Result<(), XlsxError> {
    let mut workbook = Workbook::new();

    // Add a worksheet to the workbook.
    let worksheet = workbook.add_worksheet();

    fn skip_check(value: &str) -> bool {
        value == "Peach"
    }

    //use rust_xlsxwriter::Format;
    let _foo = Format::new().set_bold();

    #[derive(ExcelSerialize, Serialize)]

    struct Produce {
        #[serde(skip_serializing_if = "skip_check")]
        fruit1: &'static str,

        fruit2: &'static str,
    }

    // Create some data instances.
    let items = [
        Produce {
            fruit1: "Peach",
            fruit2: "= Peach?",
        },
        Produce {
            fruit1: "Plum",
            fruit2: "= Plum?",
        },
        Produce {
            fruit1: "Pear",
            fruit2: "= Pear?",
        },
    ];

    worksheet.set_serialize_headers::<Produce>(0, 0)?;
    worksheet.serialize(&items)?;

    // Save the file.
    workbook.save("serialize.xlsx")?;

    Ok(())
}

screenshot

Note, fixing this may break serialization of array values in a struct. That probably isn't a common use case though.

zjp-CN commented 9 months ago

IIUC skip_serializing_if on one field here basically means to skip serialization of the row containg other fields given a condition.

So the caller can do it directly by iterator filter or continuing in the loop

let slice: &[Produce] = ...;
slice.iter().filter(|p| !skip_check(&p.fruit1)).try_for_each(|p| worksheet.serialize(p));
zjp-CN commented 9 months ago

Oh, I just realized the problem is not that callers can't make it, instead it's if they use skip_serializing_if, the outcome is incorrect 😢

jmcnamara commented 9 months ago

it's if they use skip_serializing_if, the outcome is incorrect 😢

Yes. That is it. I should have written a clearer bug report. ☺️