search

hoodie / icalendar-rs

📆 icalendar library, in Rust of course - for fun
Apache License 2.0
126 stars 34 forks source link

Let caller provide writer #11

Closed JackMordaunt closed 4 years ago

JackMordaunt commented 4 years ago

As it stands, the only way I could see to print a calendar was to use the Calendar::print method which is coupled to stdout.

Ideally the API should let the caller provide the writer, eg a file handle - which Calendar::fmt_write does. It just isn't public!

Thoughts?

hoodie commented 4 years ago

actually I seem to remember adding a ToString implementation.

matthiasbeyer commented 4 years ago

Rather a TryInto, right?

hoodie commented 4 years ago

good point, that wasn't around back than, is this enough already? otherwise I'd welcome a PR adding a TryInto<String>

strohel commented 4 years ago

Actually, adding TryInto<String> should not be needed, as Calendar already provides ToString trait with to_string() method (which is public): https://github.com/hoodie/icalendar-rs/blob/5236e106395f4024c4973c5298276ca61be73194/src/calendar.rs#L99-L106

It is used e.g. here: https://github.com/strohel/goout-calendar/blob/fb56fe5543e3ef1b70af266b5e36d659f8292be0/src/generation/mod.rs#L228 - it works well for me.

strohel commented 4 years ago

There is one thing that is not ideal though: both current ToString implementation and impl TryInto<String> for &Calendar { suggested in https://github.com/hoodie/icalendar-rs/pull/12 force one to allocate new String with the whole Calendar serialization (potentially huge). That should not be needed at all.

The solution is mentioned right in ToString trait documentation:

This trait is automatically implemented for any type which implements the Display trait. As such, ToString shouldn't be implemented directly: Display should be implemented instead, and you get the ToString implementation for free.

I.e. remove whole impl ToString for Calendar and instead add impl Display for Calendar by moving and adapting existing Display::fn fmt_write<W: fmt::Write>(&self, out: &mut W) -> Result<(), fmt::Error> {. Pluse we can get rid of now redundant /// Prints to stdout /// FIXME code repetition pub fn print.

If there's an agreement, I'll prepare PR for that, should be easy.

hoodie commented 4 years ago

@strohel how about this