Add cucumber 8.0.0 support #7
Conversation
anmarchenko
force-pushed
the
anmarchenko/cucumber8_support
branch
from
e029193
to
2d5c241
Compare
| @@ -1,5 +1,3 @@ | |||
| require "datadog/tracing/contrib/integration" | |||
There was a problem hiding this comment.
I guess there is no harm requiring it . I supposed it is currently redundant because the gem has already loaded ddtrace. This might changed in the future, need to watch out.
There was a problem hiding this comment.
I am very wary against superfluous requires... from looking at this line I get all sorts of worries:
- why do we require this again if we have
require "ddtrace"inci.rb? is this code not loaded by gem itself? - if this code is not loaded by ddtrace gem why does it even exist? is it only for other gems but not for ddtrace itself?
- if this code was loaded by ddtrace gem then why do we require it again? are we not sure that it was loaded? do we think that someone did something to this code and now we need to load this again?
and so on, and so on... Maybe I am wrong though as I don't have a lot of experience with library development :)
There was a problem hiding this comment.
of course if we stop requiring the whole ddtrace gem in the future (which we'll do) it will change - in this case the right place to require parts of ddtrace we need would be ci.rb file so that all these require are clearly visible in the library's entrypoint
| let(:cli) do | ||
| cucumber_8 = Gem::Version.new("8.0.0") | ||
|
|
||
| if Datadog::CI::Contrib::Cucumber::Integration.version < cucumber_8 |
There was a problem hiding this comment.
Version check is always feasible as long as we are awared of the version changes.
Wonder what would be your thoughts on leveraging Method#arity?
https://ruby-doc.org/core-2.5.3/Method.html#method-i-arity
There was a problem hiding this comment.
interesting idea! I honestly think that version check works better in this case and here is the reason:
- arity of the method does not say anything about the underlying implementation (the semantics and order of arguments might change but arity will stay the same)
- in this case there is clear changelog entry for Cucumber 8.0.0 that states this change so it is easy enough to refer to changelog to see when it was changed
|
@TonyCTHsu I will try to backport it to dd-trace-rb and see if it breaks there |
Appraisals for cucumber 8.0.0 and making sure tests pass