Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return partial RSS feeds, rss_loose feature, resolves #22 #23

Closed
wants to merge 6 commits into from

Conversation

msjyoo
Copy link

@msjyoo msjyoo commented Jul 10, 2016

Hello,

Here is the pull request for the partial RSS feeds feature.

  • Changed struct signatures using conditional compilation
  • Changed struct impl using conditional compilation
  • Added feature flag rss_loose to activate the changes to preserve backwards compatibility
  • Modified tests so that error tests are ignored since activation of feature flag suppresses errors
  • Added the rss_loose feature flag to the test routine
  • Documented the changes in README and rustdoc
  • Added a README Contributors section, and added myself

One thing I'm concerned about is I've had to use the feature flag #![feature(stmt_expr_attributes)] to be able to get enough control over the compiler to implement these changes, so currently this patch only works for the nightly branch until that feature is un-gated. So it might be a better idea to cook this patch until then.

Still, others might find this useful regardless by leaving it here.

Please review and comment / merge.

Thanks!

@msjyoo
Copy link
Author

msjyoo commented Jul 10, 2016

#22

@frewsxcv
Copy link
Member

I'm not planning on merging anything that breaks compatibility with stable or beta.

That said, I think you should be able to replace most of your stmt_expr_attributes usages (which is unstable) with cfg! (which is stable) without too much effort.

Thanks for the pull request!

@frewsxcv
Copy link
Member

Another resource: https://doc.rust-lang.org/std/macro.cfg!.html

@msjyoo
Copy link
Author

msjyoo commented Jul 10, 2016

@frewsxcv I've tried that of course ;) But, using if cfg!() is still indicating to the compiler that I want both if / else sections compiled, and of course since this is a type changing conditional compilation, it produces an error (since both cases has to be compiled, before one being optimised away by the true / false constant if). Took me a while to figure out that one :P

@msjyoo
Copy link
Author

msjyoo commented Jul 10, 2016

@frewsxcv I understand not accepting patches that break stable compatibility, but I'd like to leave this open in case anyone else comes across the same problem as mine (and willing to use the nightly) and also for the time when the stmt_expr_attributes becomes stable.

@msjyoo
Copy link
Author

msjyoo commented Jul 10, 2016

Tracking issue here: rust-lang/rust#15701

@jameshurst
Copy link
Member

This has been resolved by #24.

@jameshurst jameshurst closed this Sep 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants