simple_excel_writer icon indicating copy to clipboard operation
simple_excel_writer copied to clipboard

impl `std::fmt::Display` for `CellValue`

Open piccoloser opened this issue 3 years ago • 2 comments

It's happened on a few occasions that I've needed to reference the value of a cell. Would implementing std::fmt::Display for CellValue be a good idea?

A quick example of a time I've had to do this is when writing a program to format report outputs, one of which required the name of each sheet to be named after the value of a column value (eg. format!("PO Line {}", line_no); )

This solution to the issue was provided to me, in the form of a function, by a user on Reddit:

fn display_cellvalue(cellvalue: &CellValue) -> impl fmt::Display + '_ {
    struct Wrapper<'a>(&'a CellValue);
    impl<'a> fmt::Display for Wrapper<'a> {
        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
            match &self.0 {
                CellValue::Bool(b) => write!(f, "{}", b),
                CellValue::Number(n) => write!(f, "{}", n),
                CellValue::String(s) => write!(f, "{}", s),
                CellValue::Blank(_) => write!(f, ""),
                CellValue::SharedString(s) => write!(f, "{}", s),
            }
        }
    }
    Wrapper(cellvalue)
}

Calling this function and passing a reference to a CellValue will return the value in a printable form.

piccoloser avatar Feb 08 '21 16:02 piccoloser

I have a small objection the implementation proposal: returning an opaque type makes it unstorable unless boxed. Moreover, in this way the return type does not implement traits that could be useful in niche cases (like Copy). On the other hand, being an opaque type make it resilient to breaking changes, but in this case I don't think that it's worth.

TL;DR: I think that returning a concrete CellValueDisplay<'a> instead of an impl fmt::Display would be better.

dodomorandi avatar Feb 18 '21 10:02 dodomorandi

TL;DR: I think that returning a concrete CellValueDisplay<'a> instead of an impl fmt::Display would be better.

I agree with your point here; the code provided worked for me, but would definitely need to be adjusted for general use.

piccoloser avatar Feb 22 '21 20:02 piccoloser