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

Introduce bang methods which raise exceptions when API returns errors #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vbyno
Copy link
Contributor

@vbyno vbyno commented May 16, 2019

e.g.: mws_api.subscriptions.register_destination!(...)

@main24
Copy link
Member

main24 commented May 17, 2019

Basically I believe there is no reason to keep both ways, i.e. when is it really necessary not to throw an exception if a request isn't successul?

@vbyno vbyno force-pushed the bang_methods_call branch from 075789d to cda44f5 Compare May 17, 2019 06:43
@vbyno
Copy link
Contributor Author

vbyno commented May 17, 2019

Basically I believe there is no reason to keep both ways, i.e. when is it really necessary not to throw an exception if a request isn't successul?

@main24, the first thing which comes to my mind is doing several requests in the loop, or several threads. For example,

results = feed_data_array.map { |feed_data| mws_api.feeds.submit_feed(feed_data) }
if results.all? { successful? }
   ...
else 
  ...
end

This would be much harder to process if we do it via catching exceptions.

Although, having error-based method a default one wouldn't be a bad idea. soft method could just be a flag (raise_error: false). Unfortunately, I'm not sure how this huge change in API could impact Veeqo app. It would require a rechecking the existing code and refactoring it.
Also, since the library is open-sourced, to follow the best practices, it would be nice to deprecate method in a new minor version. And replace it in a new major version (just to follow semantic versioning).
^ All of this will be needed if we decide to keep only "the strict" approach

@vbyno vbyno force-pushed the bang_methods_call branch 2 times, most recently from 5607c08 to b37992e Compare June 4, 2019 13:05
@vbyno
Copy link
Contributor Author

vbyno commented Jun 4, 2019

@main24 , I've added deprication warning to the regular methods. Please, take a look

@vbyno vbyno force-pushed the bang_methods_call branch 2 times, most recently from 54c77bd to 270c3f2 Compare June 4, 2019 14:14
README.markdown Outdated
c.user_agent = 'My Seller Tool/2.0 (Language=Ruby/2.5.5)'
end
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unrelated to the PR, please extract it into separate commit.

@@ -43,19 +44,40 @@ def build_query_from_params(action, params)

def method_missing(name, *args)
if self.class::ACTIONS.include?(name)
warn "[DEPRECATION] `#{name}` will be removed in next major version. " \
"Please use `#{name}!` instead."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically we're not going to get rid of methods without bang, but will change their behavior to the same as ones with bang – they'll be raising exceptions.

@vbyno vbyno force-pushed the bang_methods_call branch 5 times, most recently from 1f40e50 to 4f11454 Compare June 5, 2019 09:43
e.g.: mws_api.subscriptions.register_destination!(...)
Deprecate old methods (with no bangs)

Update documentation
@vbyno vbyno force-pushed the bang_methods_call branch from 4f11454 to 5b5455c Compare June 5, 2019 09:59
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