-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
TwitterUserAgent Tweaks #363
Conversation
} | ||
def include_retweets? | ||
if options[:include_retweets].present? | ||
!!options[:include_retweets] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be present but "false"
. I'd suggest
def include_retweets?
options[:include_retweets] != "false"
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably a cleaner way than what I have, though same outcome.
Edit: In fact, by that regard, could I not just use the following?
def include_retweets?
!!options[:include_retweets]
end
Edit2: Nevermind, I can't because of the default being true. Done.
You should add a spec that shows it skipping older tweets. Let me know if you need assistance! |
I was originally going to write a spec for that, but then I couldn't think of a nice way to do it (since the boilerplate in the 'before' clause doesn't really apply for this test, so I didn't bother. Will make an attempt at it now, but would be happy to hear a 'best practice' way to do it. Edit: How's that? |
Looks good to me @alias1. If you're happy with it and have manually tested it with some Twitter data, go ahead and merge. Nice work! |
@alias1 are you happy with this? |
Been a bit busy with life priorities, but yup, happy with it and merged :) |
TwitterUserAgent Tweaks
Solves #357 #359