search

jmcnamara / rust_xlsxwriter

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

feature request: Make `chrono` dependency optional (or support `time` crate!) #6

Closed jacobsvante closed 1 year ago

jacobsvante commented 1 year ago

Feature Request

cargo-deny gives me an error about RUSTSEC-2020-0071 because of the chrono dependency that rust_xlsxwriter brings in.

jmcnamara commented 1 year ago

Thanks. I'll fix that in the next release.

jacobsvante commented 1 year ago

Wonderful news!

jmcnamara commented 1 year ago

Chrono issue for reference: https://github.com/chronotope/chrono/issues/602

I've pushed a fix to main for this. You can check it out when you get a chance but on my side rust_xlsxwriter is no longer reporting that issue with cargo deny.

jacobsvante commented 1 year ago

Great stuff! I didn't know that you could get rid of the time 0.1 dependency by passing in --no-default-features.

I'll try to find some time to submit a PR with time support soon, if you don't mind...?

And if so - what do you think about making chrono optional and not included by default? I personally prefer the whitelist approach taken by tokio for example.

jmcnamara commented 1 year ago

Great stuff! I didn't know that you could get rid of the time 0.1 dependency by passing in --no-default-features.

That seems to by the main suggested workaround for this issue on the Chrono GitHub issue: https://github.com/chronotope/chrono/issues/602

I'll try to find some time to submit a PR with time support soon, if you don't mind...?

You are talking about the time create, right? I did look at that as an initial option for the date/time handling but chrono seemed to be more commonly used. Could you explain a little why you think that would be better?

And if so - what do you think about making chrono optional and not included by default?

I'm open to the idea. :-)

I personally prefer the whitelist approach taken by tokio for example.

Do you have a link to that. I didn't see it in their toml files.

jmcnamara commented 1 year ago

I'm going to close this since the original issue with cargo-deny is fixed.

If you would like to circle back to discuss the time crate option at some point you can do that in a new GitHub issue.

Thanks for the report. Closing.

jacobsvante commented 1 year ago

Sorry for the late reply, busy days lately!

I'll try to find some time to submit a PR with time support soon, if you don't mind...?

You are talking about the time create, right? I did look at that as an initial option for the date/time handling but chrono seemed to be more commonly used. Could you explain a little why you think that would be better?

Yup, the time create.

Mainly I think the argument is about choice and minimizing dependencies. In your app code you will probably have chosen between time and chrono (e.g. in your Serde derived structs/enums). There are many many crates doing this already, for example postgres-types. Many big crates like tracing-subscriber use time as a dependency. Also chrono depends on time.

And if so - what do you think about making chrono optional and not included by default?

I'm open to the idea. :-)

Great, I'll try to find the time soon!

I personally prefer the whitelist approach taken by tokio for example.

Do you have a link to that. I didn't see it in their toml files.

As you can see here there are no default features: https://docs.rs/crate/tokio/latest/features.

I'm going to close this since the original issue with cargo-deny is fixed.

If you would like to circle back to discuss the time crate option at some point you can do that in a new GitHub issue.

Sounds reasonable! Thanks for taking your time.

jmcnamara commented 1 year ago

Mainly I think the argument is about choice and minimizing dependencies

I can look at supporting time and moving the existing methods to write_chrono_datetime() etc. with chrono as an optional dependency.

As you can see here there are no default features

That's neat.

jacobsvante commented 1 year ago

@jmcnamara I think the usual way that it's done is to have just one write_datetime(), which is conditionally compiled to use chrono or time, depending on if you've enabled the feature chrono or time when installing rust_xlsxwriter.

jacobsvante commented 1 year ago

(And if you have neither feature enabled you won't have a write_datetime() function.)

jmcnamara commented 1 year ago

@jacobsvante I don't know if you are still interested in this but I have made chrono an "optional on" feature in rust_xlsxwriter v0.41.0 and it will be "optional off" in future version (and current main). I've added the ExcelDateTime struct as a native alternative.

jacobsvante commented 1 year ago

Sounds great 👍 Looks like you're progressing nicely with this lib!