Upgrade experience

[c-git]

Hi,

I upgraded to version 3.0.0 yesterday (off the fork for now) because I needed the jiff support for me to continue what I was working on. I wasn't sure if you wanted feedback on how I found the upgrade experience? If not it's fine. Feel free to close the issue, there will be no hard feelings.

[MathNya]

@c-git
Thank you very much.
Version 3.0.0 is scheduled for release soon.
However, as you can see, this is a fairly major update.
I’m grateful to receive your feedback before the release.
Please do let me know what you think.
If necessary, I’ll delay the release to address any issues.

[c-git]

I will just type out my feedback from memory and if desirable I will generate a small program to demonstrate what I mean.

1. We should try to provide a change log (I personally haven't been able to invest the time yet to learn how to generate them automatically but I consume them all the time in various projects. I especially look for them when there are breaking changes. At a minimum we should probably provide a manual change log if better cannot be done (I don't have the bandwidth right now to learn how to do it). For automatic generation I know some projects use PR names, some use commit message (I think) but I haven't setup either yet. egui has documented their process and we probably don't need anything that involved but we can probably borrow steps from there to help with setting up a process for this crate (and maybe seeing what automation they have setup). https://github.com/emilk/egui/blob/main/RELEASES.md . The reason I went looking for a change was when I hit the breaking change where umya_spreadsheet::Spreadsheet changed to umya_spreadsheet::Workbook as I didn't remember what the new name was. I did remember seeing the PR after I saw the new name but took me a little while to find it.
2. The deprecated "get" functions wasn't that big a deal. A little annoying but the tooling provided good help to find what the new name was.
3. I use the anyhow error crate in my application and the change of return type from Option to Result while in principle is a good thing the Error variant returned does not implement the Error trait from the standard library which means I can't easily compose it using the context function in anyhow. Given that in this case it doesn't add much info other than not found. I just turned it back into an Option (by using (.ok()) to get my code to compile. See example below. Sorry if there are small bugs in it. I copied and pasted but modified by hand to make it easier to understand. (Really wish the project I use umya-spreadsheet on was open source so I could just link to the commit messages but it's not practical to open source due to the nature of what the code is used for).

Before:

fn load_sheet( spreadsheet: &umya_spreadsheet::Spreadsheet) -> anyhow::Result<()> {
        let sheet = spreadsheet
            .get_sheet_by_name("sheet name")
            .context("failed to open the sheet in customer spreadsheet")?;
        todo!("Load info from sheet")
}

After (.ok() added but only need as error type is &'static str):

fn load_sheet( spreadsheet: &umya_spreadsheet::Workbook) -> anyhow::Result<()> {
        let sheet = spreadsheet
            .sheet_by_name("sheet name")
            .ok()
            .context("failed to open the sheet in customer spreadsheet")?;
        todo!("Load info from sheet")
}

Number 3 was the main issue that affected me that I think should be "improved" before release. I think we should create a "proper" error type. I think there are some error types in this crate already I didn't check if they implement the Error trait. If they do we can use the same approach for this error type or we can use thiserror or manually implement the Error trait.

[MathNya]

@c-git
Thank you for letting us know.

1. I just remembered that we prepared a changelog when we released version 2.0.0.
   https://github.com/MathNya/umya-spreadsheet/blob/master/CHANGELOG.md
   That was also automatically generated.
   We’ll prepare a similar one this time as well.
   Also, I believe the only name that changed significantly was umya_spreadsheet::Workbook.
   I’ll add this to the README.md so it’s easy to spot.

2. I was worried because it was a system-wide update, but I’m glad it didn’t seem to cause too much of a burden.

3. This is the biggest problem.
   I’ll read it carefully. Please give me a moment.

[c-git]

I'll prepare a name example when I get home. Don't stress. It's easy to fix. But fixing it is a breaking change so we should do it before release.

[c-git]

Created a small example. Hope that helps make it clear. Code is also here https://github.com/c-git/umya_spreadsheet_errors_example/blob/main/src/main.rs

use anyhow::Context;

fn main() -> anyhow::Result<()> {
    create_new()?;
    read_existing()
}

/// These return `&'static str` as the error and we use `.ok()`
/// to convert them into options to be able to wrap them with anyhow
fn create_new() -> Result<(), anyhow::Error> {
    let mut book = umya_spreadsheet::new_file();
    book.new_sheet("Sheet2")
        .ok()
        .context("failed to create sheet")?;
    book.sheet_by_name_mut("Sheet2")
        .ok()
        .context("failed to find sheet")?
        .cell_mut("A1")
        .set_value("TEST1");
    Ok(())
}

/// `umya_spreadsheet::reader::xlsx::read` returns an XlsxError and we can just
/// wrap it with anyhow so we don't lose the original error information
fn read_existing() -> Result<(), anyhow::Error> {
    let path = std::path::Path::new("non existent file");
    umya_spreadsheet::reader::xlsx::read(path).context("failed to read sheet")?;

    // Prints
    // Error: failed to read sheet
    //
    // Caused by:
    //     IoError: No such file or directory (os error 2)/tests/test_files/aaa.xlsx
    Ok(())
}

[c-git]

Thanks for fixing the error type, it works with anyhow now

39b1f1a

Was pleasantly surprised to notice the change recently.

[schungx]

Just upgraded. Worked like a charm, except for missing the cell_collection method...

Also, I suppose it would be nice to expose a few iterators to scan through worksheet cells instead of always allocating a Vec...

[MathNya]

@schungx
Thank you for your feedback.
We plan to address the issue you mentioned in the next update.
To ensure we don’t make any unnecessary changes, we would appreciate it if you could provide more detailed information.

[schungx]

Thanks for the good work.

I realize that collection_to_hashmap provides a raw iterator to the collection of cells. That's enough for me at this point.

Kudos to the team for a seamless upgrade experience. I literally just replaced off all get_ and removed a bunch of & and it Just Works.