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

Rest client #follow! method rescues Twitter::Error::Forbidden #502

Closed
joshsilverman opened this issue Dec 15, 2013 · 6 comments
Closed

Rest client #follow! method rescues Twitter::Error::Forbidden #502

joshsilverman opened this issue Dec 15, 2013 · 6 comments

Comments

@joshsilverman
Copy link

Is there a reason you rescue 403 errors? I've just noticed that I have been missing a whole lot of these...

@sferik
Copy link
Owner

sferik commented Dec 15, 2013

There are two versions of the follow method: #follow and #follow! The version without the bang suppresses Forbidden errors, the version with the bang raises Forbidden errors.

@sferik sferik closed this as completed Dec 15, 2013
@joshsilverman
Copy link
Author

I tried both methods and neither version raises an error. I also have modified the issue title to make clear that the follow! is the method rescuing, not follow.

Refer to line 142 of https://github.com/sferik/twitter/blob/master/lib/twitter/rest/api/friends_and_followers.rb

You can see that you're rescuing the error in follow! and not allowing it to propagate. Did you mean to have the rescue statement in the follow method maybe?

@sferik
Copy link
Owner

sferik commented Dec 16, 2013

Sorry. My earlier post was from memory. I was posting from my phone, so it wasn’t easy for me to pull up the code. Looking at it now, you appear to be right. Note that the #follow method is implemented using the follow! method, so they both rescue Twitter::Error::Forbidden, as you say.

I’m trying to remember why I added the rescue in the first place. It was added in 7d42dd5, just before version 3.0.0 was released and has been in every version since then. You’re the first person to complain about it.

I guess my thinking was, if you try to follow someone who you don’t have permission to follow, it should fail silently instead of raising an exception. I’m not sure that was the correct decision or whether that’s even what I was thinking at the time. Maybe there’s a better reason that I can’t remember now. I’m worried that removing it now would cause exceptions for a lot of people who don’t expect it.

@sferik sferik reopened this Dec 16, 2013
@joshsilverman
Copy link
Author

No problem. I really appreciate the work you've done on this gem.

I'm having trouble coming up with a reason why it would be helpful to silence these errors. Wouldn't you expect #follow! to bark loudly if there was a permission error? Is it possible that you were rescuing attempted follows of suspended users?

I have implemented a workaround in my code: I now throw an error whenever #follow.empty? == true. That state indicates vaguely that something unexpected happened, which is enough of a warning system for me.

In terms of what's in the best interest of the community of users of the gem, I suspect that a large volume of follows are failing silently ( I believe that this has been a non-trivial issue in our codebase for as much as 10 months -- I don't know how we missed it). I think it would be best to air on the side of exposing the full Twitter response to users, especially for #follow!.

ps. I agree that it's strange that nobody else is complaining about this. Another clue may be that I'm seeing these errors when I hit a follow limit. The exception still seems valuable in that case.

@sferik
Copy link
Owner

sferik commented Dec 16, 2013

I suppose I’d be willing to make this change in an upcoming minor release. Technically, it doesn’t change any public APIs.

@joshsilverman
Copy link
Author

Thanks Erik -- I think it's in the best interest of users.

I would bet that you'll confuse some people who thought their follows had been going through, but ultimately they will appreciate the more transparent behavior.

@sferik sferik closed this as completed in b949c04 Dec 17, 2013
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

No branches or pull requests

2 participants