twingly / twingly-search-api-ruby

:gem: Twingly Blog Search API in Ruby
https://developer.twingly.com/
MIT License
4 stars 0 forks source link

Improve time handling #51

Closed dentarg closed 8 years ago

dentarg commented 8 years ago

What do we think about this?

jage commented 8 years ago

Looks good, I'm thinking maybe should think about what happens when a non-compliant object passed on. At least document with a spec that it will blow up, but it might be even better to do an assertion in the setter and raise a more helpful error.

Something like:

    context "when given non-time object" do
      let(:time) { "2013-12-28+09%3A01%3A22" }

      it "should raise exception" do
        expect { subject.request_parameters }.to raise_error(NoMethodError)
      end
    end
dentarg commented 8 years ago

I like your suggestion, with the modification: raise_error(QueryError, "Not a Time object") instead of NoMethodError.

Now I'm think if we should bring back the setters, so we can raise immediately when you assign start_time or end_time, or if it's good enough to get the exception when you do query.execute (which calls #request_parameters, which you are not likely to call yourself)

jage commented 8 years ago

Now I'm think if we should bring back the setters, so we can raise immediately when you assign start_time or end_time, or if it's good enough to get the exception when you do query.execute (which calls #request_parameters, which you are not likely to call yourself)

Agree, I think it's better with assertions in the setter. Better for the user (the stack trace will be easier to understand) and easier to test (no prepping).

dentarg commented 8 years ago

@jage what do you think of the latest changes?

jage commented 8 years ago

LGTM!