search

twingly / twingly-search-api-ruby

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

Automatically convert Query#start_time and #end_time to UTC #46

Closed roback closed 8 years ago

roback commented 8 years ago

Noticed that the documentation above start_time= does not show up in the yard documentation, but at least its documented in the code. Found an issue on yard for that: https://github.com/lsegal/yard/issues/516 but it doesn't look as it will be fixed :(

close #43

dentarg commented 8 years ago

setter= documentation is hidden if used with attr_accessor or attr_reader

so if we remove our attr_readers it wont be hidden?

dentarg commented 8 years ago

so if we remove our attr_readers it wont be hidden?

yes, first thing mentioned in the issue, sorry :)

dentarg commented 8 years ago

IMHO we want to the documentation to not be hidden, not using two attr_readers seems like a small tradeoff we can handle

roback commented 8 years ago

IMHO we want to the documentation to not be hidden, not using two attr_readers seems like a small tradeoff we can handle

I agree, I haven't tested the things suggested in the issue yet, but I'll do that.

dentarg commented 8 years ago

I think we are happy now :) Good work!

walro commented 8 years ago

Nice!

roback commented 8 years ago

Im not sure if this is a breaking change or not. It is a breaking change for the users that (wrongly?) used the client with non-UTC times, as they will receive another result when making a query now. The users that converts to UTC themselves will still get the same result back from the api now, as they did before.

roback commented 8 years ago

It is a breaking change for the start_time and end_time getters though.

walro commented 8 years ago

I think we could call it a bug-fix actually, so bumping the last digit is fine for me.

walro commented 8 years ago

It is a breaking change for the start_time and end_time getters though.

Ah, true.

dentarg commented 8 years ago

It is a breaking change for the start_time and end_time getters though.

That they now always return the time in UTC?

roback commented 8 years ago

That they now always return the time in UTC?

Yes

dentarg commented 8 years ago

Let's :shipit:?

roback commented 8 years ago

Yes!

jage commented 8 years ago

It is a breaking change for the start_time and end_time getters though.

Agree, I think the breaking change is the to_time though, if you pass in an object and then want to compare with #end_time or #start_time it will fail since the getter might return another object than the setter received.

[7] pry(main)> d = DateTime.new
=> #<DateTime: -4712-01-01T00:00:00+00:00 ((0j,0s,0n),+0s,2299161j)>
[8] pry(main)> d.to_time == d
=> false

UTC "should not" break anything since the time objects can be compared/transformed.

[1] pry(main)> t = Time.now
=> 2016-02-15 21:15:40 +0100
[2] pry(main)> t == t.utc
=> true

In #15 we ensured that we would get the same object out as we put into the start_time/end_time. That allowed users of the gem to easier compare the query dates with the result dates. Now they might be mutated if they aren't Time objects.

Extra:

Interesting that we use Time here but Post#indexed and Post#published return DateTime objects https://github.com/twingly/twingly-search-api-ruby/blob/750df991f0dc96405e0e96dcf6eb1805b82ccad2/lib/twingly/search/post.rb#L35-L36