search

thscharler / spreadsheet-ods

Apache License 2.0
29 stars 6 forks source link

Compression level configuration #55

Closed Shatur closed 2 months ago

Shatur commented 3 months ago

I faced a performance issue after update and thought that it was a bug in zip dependency. But it turns out that the compression level is changed (see https://github.com/zip-rs/zip2/issues/138#issuecomment-2171627141). Could you add configuration for it?

thscharler commented 2 months ago

I added OdsWriteOptions similar to the existing OdsOptions for reading.

Can you give this a try, then I will release a patch.

4a56a0e

Shatur commented 2 months ago

@thscharler could you re-export zip::CompressionMethod from the crate?

thscharler commented 2 months ago

rather not. had to much trouble with color-rs.

I'll look at this.

Shatur commented 2 months ago

But why? Otherwise I will need to include zip directly in order to set the compression level.

thscharler commented 2 months ago

Yes, and if you look at the compression level there are quite a lot of them that are only active for certain features.

I don't want to duplicate all of those features in spreadsheet-ods just to support those cases. And if I set those features for in my crate they are active for your whole application. I've heard that is seen as bad form.

For the mentioned color-rs: I've reexported just the Color, and then some upstream crate (num_traits) 2 levels away broke the color-rs crate. To make everything work again I had to add num_traits with a specific version to my crate. I'm sure that will bite me in the ass coming time.

That's why I think it's better if you use zip directly in your crate if you have special requirements.

Shatur commented 2 months ago

But you don't need to export each compression method and add each feature, I just talking about CompressionMethod enum.

Usually it's a good idea to export types that are used in public API. Otherwise user can accidentally set too new or too old version and it won't compile :(

Shatur commented 2 months ago

Also spreadsheet-ods needs only these features: https://github.com/thscharler/spreadsheet-ods/blob/ad4cb32073f7bfef0c7d3f311267ba68d43bcb13/Cargo.toml#L79 So as a user, I have to put the same to my Cargo.toml if I just want to change the compression level (without pulling all extra default deps I don't care about).

thscharler commented 2 months ago

You have a point here. I think I was too tired to think straight.

Shatur commented 2 months ago

Thank you!