search

rust-syndication / rss

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

Add a default builders feature that can be disabled #83

Closed SSheldon closed 4 years ago

SSheldon commented 4 years ago

The rss crate depends on derive_builder to generate convenient builders for creating the contents of an rss feed. However, for users that only want to parse an rss feed, not create their own, this results in 12 additional dependencies, some of which are pretty heavy.

This change adds a builders feature to the crate which is enabled by default. When disabled, though, none of the builders are included in the crate, and the derive_builder dependency is omitted. This should be a backwards-compatible way that users can opt for having lighter dependencies.

While more features and cfg attributes always add complexity to code, I think this comes out pretty manageable. I added a step to the travis tests to ensure that the crate compiles with and without the feature.

SSheldon commented 4 years ago

This PR makes the same change as one I've opened for the atom_syndication crate, rust-syndication/atom#19.

SSheldon commented 4 years ago

Sorry, silly question about y'all's review workflow: does approved but not merged mean anything special? Do you always have one person approve and a second person do the merge?

No rush and this change isn't urgent, I just haven't known whether getting an approval but no merge means I should do something, ping someone, or just keep waiting :)

Thanks for taking the time to review my PR and for your work maintaining these crates!

DCjanus commented 4 years ago

To be honest, as I know, there is no workflow for this, I approve this PR because it 'LGTM', but I hope there shoule be another reviewer before merge.

Anyway, I think this PR wouldn't be better, I'd like to merge this.

DCjanus commented 4 years ago

Thanks for everything you've done

SSheldon commented 4 years ago

Appreciate the merge! I wasn't trying to pressure you into merging :) I'm happy to tag some of your co-maintainers in future PRs, too, if we want to get another review!