search

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

Return partial RSS feeds #22

Closed msjyoo closed 8 years ago

msjyoo commented 8 years ago

Hello,

You're using Option() for the RSS channel and item fields, but you fail the parse function when for example, the Channel Description is missing. I don't see why this should be necessary?

Why not simply set Option(None) and continue on with the parsing returning the partial results?

I've come across a site that this library refuses to work on (looking at you abc.net.au -_-) because I get a Err(ChannelMissingDescription).

Thanks!

frewsxcv commented 8 years ago

The RSS specification says that description is required:

http://cyber.law.harvard.edu/rss/rss.html#requiredChannelElements

So, as per the specification, their RSS feed is not valid. What do you propose I do?

frewsxcv commented 8 years ago

It's also required in the RSS 1.0 spec (the link above is the RSS 2.0 spec)

http://web.resource.org/rss/1.0/spec#s5.3.3

msjyoo commented 8 years ago

I understand that abc.net.au's feeds aren't RSS compliant, but if the library fails just because the given feed isn't strictly RSS compliant, it wouldn't be of much use in real life. Many websites are lazy and don't strictly follow the spec.

Since you are already using Some() on those fields, wouldn't it be as trivial as letting the value be None if the description or any other fields aren't detected?

msjyoo commented 8 years ago

Otherwise, what is the point of using the Result struct in the first place? It's only useful if the value can be omitted. Let me know what you think.

frewsxcv commented 8 years ago

Otherwise, what is the point of using the Result struct in the first place?

Which Result struct are you referring to?

msjyoo commented 8 years ago

@frewsxcv My bad, it was https://doc.rust-lang.org/std/option/enum.Option.html

frewsxcv commented 8 years ago

So if we want this to happen, there needs to be two parsers:

Each of these parsers will result in a different struct. The strict parser should not have all Option fields.

What do you think?

frewsxcv commented 8 years ago

If it is the case that sounds good, I'm going to disclaim that I'm pretty busy right now and probably won't get around to implementing it anytime soon. If you do though, I'll very happily merge in patches.

msjyoo commented 8 years ago

@frewsxcv I'll give it a go :)

msjyoo commented 8 years ago

@frewsxcv Would it be okay to use conditional compilation to implement this? A new feature flag of rss_loose compiles the loose version of rss? Which, of course, defaults to false.

It's just that creating a separate struct of RssLoose results in a lot of code duplication (at least from what Rust I know).

    #[cfg(not(feature = "rss_loose"))]
    pub title: String,
    #[cfg(not(feature = "rss_loose"))]
    pub link: String,
    #[cfg(not(feature = "rss_loose"))]
    pub description: String,

    #[cfg(feature = "rss_loose")]
    pub title: Option<String>,
    #[cfg(feature = "rss_loose")]
    pub link: Option<String>,
    #[cfg(feature = "rss_loose")]
    pub description: Option<String>,

Thanks :)

frewsxcv commented 8 years ago

Great idea, sounds good

jameshurst commented 8 years ago

This should be fixed by #24. Any missing fields thats are declared as "required" by the RSS 2.0 Specification will default to an empty string.