-
-
Notifications
You must be signed in to change notification settings - Fork 50
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 a Duration object #71
Conversation
For results which do not have an actual duration (skipped, undefined and pending) instead hold an null object. Client can either use the "null duration" (0), or check if it is an actual duration or an null duration.
\o/ |
describe Result::Duration do | ||
subject(:duration) { Result::Duration.new(10) } | ||
|
||
it "exist? returns true" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to use nil?
here? (obviously the opposite way around to this exist?
method). That way we're not inventing a new protocol for null objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is best to adhere to the standard protocol. In the back of my head I remember seeing the construct isNull()
in relation to null objects, but I can tell you that there a lot more search result for having the null object quacking like a real object, than about naming the query to distinguish the null object from a real object.
Fixed in cc5d1cb and cucumber/common@7b1d1d9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your take-away from that @tooky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattwynne sending .nil?
or .exist?
is really a type check disguised as a message send.
The example where I've seen it used, something like:
unless duration.exist?
formatter.add_duration duration.duration * 10 ** 9
end
I think perhaps we could do something like:
duration.with_duration do |milli_secs| # I'm guessing this is what we are storing duration in?
formatter.add_duration milli_secs * 10 ** 9
end
The standard duration could then yield the value to the block, and the unknown duration could be a no-op.
WDYT? /cc @brasmusson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine if #tap
yields the integer (ms) value, or doesn't yield for the null object. Don't you agree @tooky?
If we wanted to make it crystal-clear, we could call it #tap_value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattwynne @brasmusson I think it would be a bit weird to yield the value, would it violate the POLS not to get the actual object?
#tap_value
is ok... but how about we give the value a good name? Is it recorded in milli seconds?
Then you could do:
duration.tap { |duration| formatter.add_duration duration.mill_secs * 10 ** 9 }
(Incidentally, should the duration deal with time conversions?)
@brasmusson I think that UnkownDuration should implement the same interface as a duration, but perhaps rather than return 0, it could return itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed @tooky - #tap should yield the object itself. A good name for the method is either #ms
or #milliseconds
IMO - no need for underscores.
It seems to me that with this API, bug-free code would never call that method on the null object, so it doesn't really need to implement it. Or it could throw an error telling the client to use the #tap
trick. I think returning self might be a bit surprising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duration is actually recorded in nano seconds so #nanoseconds
could be the method name. As a side note, the gherkin_formatter_adapter converts the duration to seconds, and then the gherkin::json_formatter converts it back to nanoseconds.
If we look at the duration object API as #nanoseconds
is only allowed to be called inside a #tap
block, than throwing an error is reasonable.
If we look at #nanoseconds
as a regular method of the API, then also UnkownDuration should return something reasonable for it. 0 is kind of the most neutral integer value (would it have been a float, then Float::NAN
could be used). Returning self is possible, if UnkownDuration also use some other tricks from http://devblog.avdi.org/2011/05/30/null-objects-and-falsiness/, so it responds reasonable "nullish" to the methods that could be sent to an integer:
def method_missing(*args, &block)
self
end
def to_a; []; end
def to_s; ""; end
def to_f; 0.0; end
def to_i; 0; end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With 47ec1e1 and cucumber/common@00e9701 #tap
and #nanoseconds
are used to pass the duration value to the JSON formatter, unless its an UnkownDuration
.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
The main purpose of using a pull request for this change is to have one entry point for the discussions leading up to it: cucumber/common@c6535ce#commitcomment-7953669, and on the first commit on this branch, see below.
Detailed description:
Introduce a Duration object in Cucumber::Core::Test::Result
For results which do not have an actual duration (skipped, undefined and pending) instead hold an null object. Client can either use the "null duration" (0), or check if it is an actual duration or an null duration.