jmcnamara / rust_xlsxwriter

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

feature request: row/column walkers (would you accept a PR?) #109

Closed Expurple closed 2 months ago

Expurple commented 3 months ago

Feature Request

I have some hepler types to abstract away iteration over row/column numbers:

let mut walker = RowWalker::new(&mut worksheet, 2);
walker.at_next_row(|mut w| {
    assert_eq!(w.row(), 2);
    w.write("A3")?;
    w.write("B3")
})?;
walker.at_next_row(|mut w| {
    assert_eq!(w.row(), 3);
    w.write("A4")?;
    w.write("B4")
})?;
assert_eq!(walker.next_row(), 4);

This isn't the entire API, but you get the point.

I'd like to publish and easily reuse them. But a separate library crate would require a lot of maintainance, since rust_xlsxwriter is (currently) semver-unstable. I'd have to define a range of versions, test against these multiple versions and frequently evaluate and bump the upper bound as new versions are released. (Basically, I'm reiterating my semver complaints from here)

Do you mind absorbing these heplers into rust_xlsxwriter itself? This shouldn't require much maintenance on your part, because the core write/write_with_format APIs don't break that often. And this would probably help with discoverability.

jmcnamara commented 3 months ago

Thanks for this effort. That looks like a nice interface. I've had occasional requests for the Python version of xlsxwriter to have a last_row() feature so that users don't have to track their row count.

However, I'm not in favour of adding syntactic helpers unless it is frequently requested and it feels like something I would use and want to maintain. So I will pass on this for now.

I think it would be worth wrapping it in to a crate though to gauge the level of interest in additional abstractions.

I'd like to publish and easily reuse them. But a separate library crate would require a lot of maintainance, since rust_xlsxwriter is (currently) semver-unstable. I'd have to define a range of versions, test against these multiple versions and frequently evaluate and bump the upper bound as new versions are released. (Basically, I'm reiterating my semver complaints from here)

I meant to ask at the time what your concerns were. Maybe you could explain a bit more here. Personally I don't have an issue updating the rust_xlsxwriter dependencies periodically and most of the dependencies aren't sermver stable. But I'm keen to learn what other crate authors do.

Also, from my point of view it isn't worth supporting a range of rust_xlsxwriter versions. I would prefer that users moved to the latest version (as much as possible). At least until v1.0.0. Most v0.x.0 versions of rust_xlsxwriter have significant new features.

Expurple commented 3 months ago

Ok, I gave this some thought, and actually I don't have a practical need for a range of versions. I'm always using the latest in my project. So the rant about semver turns out to be theoretical/esthetical. Just regularily bumping to the latest isn't "a lot of maintainance".

Expurple commented 3 months ago

I've just remembered another unrelated issue that I actually face with my wrappers. Manually wrapping Worksheet's per-cell methods is really annoying! My CellWalker<'_> (the inner w in the original example) has all these methods just to wrap the core write/write_with_format API:

pub fn write<T>(&mut self, data: T) -> Result<(), XlsxError>
pub fn write_with_format<T>(&mut self, data: T, format: &Format) -> Result<(), XlsxError>

// Escape hatches for random access
pub fn write_at<T>(&mut self, column: ColNum, data: T) -> Result<(), XlsxError>
pub fn write_with_format_at<T>(&mut self, column: ColNum, data: T, format: &Format) -> Result<(), XlsxError>

where
  T: IntoExcelData

And it would need even more methods to cover other APIs which I haven't used yet. But all it's doing is essentially just a "partial application" of row and column numbers to a Worksheet!

What do you think about changing (or duplicating) your API from

worksheet.write(row, col, data)?;

to something like this?

worksheet.cell(row, col).write(data)?;

It's a bit more verbose, but this would allow me to have just two &mut methods on my CellWalker<'_> and cover the entire upstream API in a future-proof way:

pub fn at_next_col(&mut self) -> MutCell<'_>

// Escape hatch for random access
pub fn at_col(&mut self, column: ColNum) -> MutCell<'_>

where MutCell<'_> is basically just this partially-applied (&mut Worksheet, RowNum, ColNum).

I think, this would also make the Worksheet API more modular and easier to navigate. You'd have per-worksheet operations and range/cell accessors on Worksheet, then per-range operations on MutRange<'_> and per-cell operations on MutCell<'_>. I've definitely had troubles with Worksheet API feeling large and hard to navigate.

Expurple commented 3 months ago

And coming back to the topic of upsteaming my wrappers. Currently, they take just 110 LOC (with insufficient docs, testing and Worksheet API coverage, ofc). With the proposed MutCell<'_> API, they should become really slim, easy to maintain, and worth simply upstreaming IMO.

Here's the entire likely API:

impl<'s> RowWalker<'s> {
  pub fn new(sheet: &'s mut Worksheet, initial_row: RowNum) -> RowWalker<'s>;

  pub fn next_row(&self) -> RowNum;

  pub fn at_next_row<T, F>(&mut self, f: F) -> T
  where
    F: FnOnce(CellWalker<'_>) -> T;

  // Escape hatches:

  pub fn at_row<T, F>(&mut self, row: RowNum, f: F) -> T
  where
    F: FnOnce(CellWalker<'_>) -> T;

  pub fn worksheet(&mut self) -> &mut Worksheet;
}

impl<'s> CellWalker<'s> {
  pub fn row(&self) -> RowNum;

  pub fn next_col(&self) -> ColNum;

  pub fn at_next_col(&mut self) -> MutCell<'_>;

  // Escape hatches:

  pub fn at_col(&mut self, column: ColNum) -> MutCell<'_>;

  pub fn worksheet(&mut self) -> &mut Worksheet;
}
jmcnamara commented 2 months ago

to something like this?

worksheet.cell(row, col).write(data)?;

That is a nice interface. It is actually similar to the way the Python OpenPyXL library (not my Python library) handles things.

However, I think that it too late to retrofit that into rust_xlsxwriter. As such I am going to close this request. Please let me know when your crate is upstream.