EasyPost/easypost-ruby

Have I/O methods return True or False depending on outcome.

Closed this issue · 4 comments

I'd like to propose that I/O methods to your API return True or False depending on the outcome and then store the error on the object itself just like ActiveRecord. This is mostly to clean up Rails application code a bit.

For example with Easypost::Shipment#buy I currently have to do the following to handle thrown errors:

def purchase_shipping
  error_json = {}
  begin
    shipment = @order.shipment
    shipment.buy(rate: {id: @order.shipping_rate[:id]})
  rescue StandardError => error
    error_json = {easypost: ["Order was placed but shipping label could not be purchased.","#{error}"]}
  end
  return render json: error_json, status: :bad_request if error_json != {}
  render shipment.as_json
end

I would much prefer to do the following:

def purchase_shipping
  shipment = @order.shipment
  return render shipment.errors.as_json if !shipment.buy(rate: {id: @order.shipping_rate[:id]})
  render shipment.as_json
end

Even for use not within a Rails app I think this would greatly clean up error handling code in any project.

att14 commented

The errors that we raise have much more information than just whether or not the call was successful. Many of our customers rely on the messages we return.

I understand that; I rely on the messages as well. My comment isn't on the messages themselves but how to access them. You use general ruby exceptions which leads to (IMO) ugly looking error handling code as I've shown in my code above. I would prefer to have it more like Rails and ActiveRecord. Please re-read my comment and let me know what you think.

EDIT: So for example if shipment.buy returned false I could then check shipment.errors to get a list of all the errors that occurred. As shown in my code above:

return render shipment.errors.as_json
att14 commented

I think that the code looks "ugly" because you are used to rails, but in most other languages this is the standard. You can also clean it up by rewriting it like:

begin
  @order.shipment.buy(...)
rescue EasyPost::Error => error
  render json: ..., status: error.http_status
else
  render json: @order.shipment.as_json
end

This change is also not backwards compatible and would require pretty much an entire rewrite of this library which we do not have the time for now. I will keep this in mind if we ever decide to rewrite this.

That's a fair answer and that's all I was looking for. I know this is a standard way of handling exception errors in other languages. I can't presume to know how your clients use this library but since most e-commerce is user driven from a website I think a lot of Rails developer would be using this library and that anyone using this from a pure standalone CLI would be in the minority. This is however pure speculation on my part with entirely no evidence to back it up.

I appreciate your response!