rust-syndication / rss

Library for serializing the RSS web content syndication format
https://crates.io/crates/rss
Apache License 2.0
419 stars 52 forks source link

Consider making fields public #71

Closed SSheldon closed 4 years ago

SSheldon commented 5 years ago

The fields of each parsed RSS element are private, only accessible through getter & setter methods. This was a change made in #35; prior to this, in version 0.4 and below, the fields were publicly accessible, like:

pub struct Category {
    /// The name of the category.
    pub name: String,
    /// The domain for the category.
    pub domain: Option<String>,
}

Looking over that pull request, I couldn't find any specific justifications for removing public access, so I'm curious if bringing back public access would be considered.

Getters and setters make sense to me over direct access when direct access could allow users to violate some invariants of your code. None of the getters/setters here seem to be used for that, though; each setter just directly sets the given value, and all invariants are just enforced by the types. Getters and setters would also make sense if you want to abstract away the internal representation of these types, but the representation of all these RSS elements has been very stable and the getter/setters are coupled pretty tightly with their current representations.

Direct public access gives users of this crate a bit more flexibility. For example, with a previous version of this library, I was able to consume RSS items and convert them to another type without additional allocation, like:

struct Entry {
    title: String,
    content: String,
}

impl Entry {
    fn from_rss(item: rss::Item) -> Self {
        let rss::Item { title, description, .. } = item;
        Entry {
            title: title.unwrap_or(String::new()),
            content: description.unwrap_or(String::new()),
        }
    }
}

With the current getter/setters approach, even if I consume an Item, I still have to clone its fields.

Making the fields public should be backwards compatible; all the getters and setters can remain as convenience. If the rust-syndication group would be comfortable with this change, I'd be happy to send a pull request!

SSheldon commented 5 years ago

It looks like there was some potentially relevant discussion about getters/setters in #41.

DCjanus commented 5 years ago

I tend to expose all fields and remove getter/setter builder. I don't know what other people think. @frewsxcv

Because macro is evil :wink:

nivekuil commented 4 years ago

Expressing my support for this change. I actually use a local fork with pub fields on Channel because borrowing the whole thing for one field was getting annoying.