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

Provide fail-fast bang equivalents for each API call #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thbar
Copy link

@thbar thbar commented Jul 28, 2012

A first pull-request after our discussion at #4.

Not completely happy with the current code, but it works though! Feel free to change whatever suits you.

I can blindly call add_emails! and I will get an exception if the list does not exist, or delete_list! will raise an error if the list does not exist as well.

@JEG2
Copy link
Collaborator

JEG2 commented Jul 28, 2012

I would like to get away from manually specifying all of the bang method variants. Could we read the public method list and wrap all of those or some similar trick?

@thbar
Copy link
Author

thbar commented Jul 28, 2012

I thought about that but wanted to have your opinion on the first stab already!

I can have a look at that, maybe with some filtering.

This assumes all the declared methods are API calls, which is the case
today. Otherwise some filtering will be needed.
@thbar
Copy link
Author

thbar commented Jul 28, 2012

Here's an updated version with less manual work. I added a test to verify the behaviour too.

@JEG2
Copy link
Collaborator

JEG2 commented Jul 31, 2012

Closer to what I had in mind yeah, but can we use an extended() (or included()) hook to avoid the need for bang_method() altogether. Or alternately, we could just define a method_missing() that forwards to the non-bang methods. Thoughts?

@thbar
Copy link
Author

thbar commented Jul 31, 2012

Well at this point, I would either leave it that way, or use extended/included, or method_missing, it's fairly equivalent to me given the size of the gem.

Let me know what you prefer and I'll do that!

I'll also add some specs around the verify! behaviour.

@thbar
Copy link
Author

thbar commented Aug 7, 2012

Hey @JEG2, any input on that? Let me know and I'll finish it!

@JEG2
Copy link
Collaborator

JEG2 commented Aug 7, 2012

Sorry. I have been busy.

The thing I don't like about the current version is that you purposefully had to move the call to bang_method() to after all the methods definitions. I would rather not need to worry that we keep it in right place as we go forward.

For that reason, I think I would prefer:

  1. Define method_missing()
  2. When called, check to see if it's a bang variant of what we already support
  3. If so, forward with error tossing
  4. If not, delegate to super
  5. Also override respond_to_missing() (http://robots.thoughtbot.com/post/28335346416/always-define-respond-to-missing-when-overriding)

This seems more maintenance friendly.

Thoughts?

@thbar
Copy link
Author

thbar commented Aug 7, 2012

Sure, no worry on the delay really! It's just that I may have a bit of spare time this week-end so I wanted to see if I could have your input before.

All that seems more maintenance friendly, absolutely; I'll update my branch with that.

Thanks!

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.

2 participants