Skip to content

Conversation

@roback
Copy link
Member

@roback roback commented Feb 9, 2016

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: lsegal/yard#516 but it doesn't look as it will be fixed :(

close #43

@dentarg
Copy link
Contributor

dentarg commented Feb 9, 2016

#setter= documentation is hidden if used with attr_accessor or attr_reader

so if we remove our attr_readers it wont be hidden?

@dentarg
Copy link
Contributor

dentarg commented Feb 9, 2016

so if we remove our attr_readers it wont be hidden?

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

@dentarg
Copy link
Contributor

dentarg commented Feb 9, 2016

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

@roback
Copy link
Member Author

roback commented Feb 9, 2016

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.

subject.pattern = "semla"
subject.start_time = time
end

Copy link
Contributor

Choose a reason for hiding this comment

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

should we also include tests like this? expect(subject.start_time.utc?).to be(true) or is it too redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not. I don't think they need their own it block though. I'll add them to the "should convert to UTC" block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure

end

context "when given time in UTC" do
let(:time) { Time.new(2016, 2, 9, 9, 01, 22, "+00:00") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this really matters, but

irb(main):003:0> Time.new(2016, 2, 9, 9, 01, 22, "+00:00").utc?
=> false
irb(main):004:0> Time.new(2016, 2, 9, 9, 01, 22, "+00:00").utc.utc?
=> true

Maybe we should use Time.parse? Then it's possible to use "UTC" (which seems what Ruby needs to interpret something as UTC)

irb(main):014:0> Time.parse("2016-02-09 09:01:22 +0000").utc?
=> false
irb(main):015:0> Time.parse("2016-02-09 09:01:22 UTC").utc?
=> true

Ruby doesnt think that Time.parse("... +00:00").utc? is true.
Since Time.new cannot take "UTC" as argument i switched so now all
dates in the query test uses Time.parse instead.

see #46 (comment)
@dentarg
Copy link
Contributor

dentarg commented Feb 10, 2016

I think we are happy now :) Good work!

end
end

context "when given time not in UTC" do
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to change, but "when given non-UTC time" has better flow imo :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think so too, but I can live with this 😄

@walro
Copy link
Contributor

walro commented Feb 10, 2016

Nice!

@roback
Copy link
Member Author

roback commented Feb 10, 2016

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
Copy link
Member Author

roback commented Feb 10, 2016

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

@walro
Copy link
Contributor

walro commented Feb 10, 2016

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

@walro
Copy link
Contributor

walro commented Feb 10, 2016

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

Ah, true.

@dentarg
Copy link
Contributor

dentarg commented Feb 10, 2016

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

That they now always return the time in UTC?

@roback
Copy link
Member Author

roback commented Feb 10, 2016

That they now always return the time in UTC?

Yes

@dentarg
Copy link
Contributor

dentarg commented Feb 10, 2016

Let's :shipit:?

@roback
Copy link
Member Author

roback commented Feb 10, 2016

Yes!

roback added a commit that referenced this pull request Feb 10, 2016
Automatically convert Query#start_time and #end_time to UTC
@roback roback merged commit 38e1afc into master Feb 10, 2016
@roback roback deleted the fix/43/convert-time-to-utc branch February 10, 2016 12:24
@dentarg dentarg mentioned this pull request Feb 15, 2016
@jage
Copy link
Contributor

jage commented Feb 15, 2016

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

@published = DateTime.parse(params.fetch('published'))
@indexed = DateTime.parse(params.fetch('indexed'))

@dentarg dentarg mentioned this pull request Feb 17, 2016
dentarg added a commit that referenced this pull request Feb 17, 2016
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.

Query#start_time and #end_time should be converted to UTC

4 participants