Implement serde::Serialize and serde::Deserialize for rss::Item about rss HOT 6 CLOSED

merged in #55

from rss.

I'm -1 personally, but I don't feel strongly. In my opinion serializing RSS to JSON is not a common enough usecase to warrant the extra dependency. I also don't want to give people the impression that they can use https://github.com/serde-rs/xml to output valid RSS XML. What do you think @jameshurst ? Another option would be to make it an opt-in optional dependency.

from rss.

Opened a pull request with opt-in serialization: #55

Let me know how it looks. You can use it by specifying features = ["serialization"] in your cargo.toml

from rss.

I'd agree with @frewsxcv here and say that this wouldn't really be a common enough use case and would probably add confusion regarding have serde output xml. I'd be -1 as well for this.

from rss.

So, the Rust API guidelines say that all types "that play the role of a data structure" should derive these. That seems a fairly strong recommendation. If the rust-syndication crate want to counter this advice, it would be good to start a discussion as part of the API guidelines repo to have some accepted exceptions transcribed in the guidelines.

I'm not sure the argument brought forward here ("we already serialize as XML natively") is very strong. The serde traits can cover anything from JSON to bincode, and there many reasons using one of those might be more attractive than XML for some transport mechanisms or a quick local cache (as we're doing in planetrs -- https://github.com/Vagdish/planetrs/issues/19). In addition, the cost here seems very low, especially as a feature that's disabled by default (ideally enabled with the serde flag, as recommended in the guidelines).

from rss.

@djc after letting this settle in my head for some months, and after reading your last comment here, i think you're absolutely right. i just updated the serialization pull request to just use the serde flag name. i'll flag you for a review if you've got a minute

from rss.