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

Handle extra API attributes with a warning. #5

Merged
merged 1 commit into from
May 4, 2011

Conversation

caius
Copy link
Contributor

@caius caius commented May 3, 2011

Rather than die with a NoMethodError, just #warn about trying to set a non-existent attribute instead.

Rather than die with a NoMethodError, just #warn about trying to set a non-existent attribute instead.
@EmmanuelOga
Copy link
Owner

Hi there. Not sure why are you doing that. Where are those extra API parameters coming from? Are you trying to add your own attributes to the objects, or are you getting extra parameters from campfire (i.e., is any of the firering classes outdated?)

@caius
Copy link
Contributor Author

caius commented May 4, 2011

They've added an extra attribute to the user json (avatar_url), which currently causes firering to crash out with a NoMethodError for avatar_url=. Idea of this commit is to stop firering crashing in the time between new attributes being added and firering being updated. Coding defensively when something that will change beyond your control is a good thing in my opinion.

@wjessop
Copy link

wjessop commented May 4, 2011

EmmanuelOga, check out the API docs http://developer.37signals.com/campfire/users.

https://github.com/EmmanuelOga/firering/blob/master/lib/firering/instantiator.rb#L12-15 is broken because it makes assumptions about the format of the data we return, and we added a field.

@EmmanuelOga
Copy link
Owner

Being defensive is important, as it is being correct. That's why I was asking what was the underlying cause of you wanting to add that code. Looks good to me, I'll add it and change a little bit the wording. I'll change it too to use the built in logger.

EmmanuelOga added a commit that referenced this pull request May 4, 2011
Handle extra API attributes with a warning.
@EmmanuelOga EmmanuelOga merged commit 0bd3b3f into EmmanuelOga:master May 4, 2011
@EmmanuelOga
Copy link
Owner

btw, just released this together with the avatar_url fix as 1.1.0.

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