Deprecate calling #ab_seen with alternative instance #317
Conversation
This test involves resetting Vanity, which sets a new logger at the default level. This means that the legacy configuration method it supports prints a deprecation to the test output. We can capture the IO and assert the presence of a deprecation message instead using minitest's #capture_io, setting a new logger within the captured block.
urbanautomaton
force-pushed
the
deprecate-ab-seen-with-alternative-instance
branch
from
7848975
to
1378e9a
Compare
| def ab_seen(experiment, identity, alternative) | ||
| if ab_assigned(experiment, identity) == alternative.id | ||
| alternative | ||
| def ab_seen(*args) |
There was a problem hiding this comment.
Hm, what do you think about keeping the arguments expanded, e.g. experiment, identity, alternative_id or experiment, identity, alternative_or_id? Condensing down to the splat might make the documentation harder to follow.
There was a problem hiding this comment.
Ah, good point - I did it this way because I wanted to avoid shadowing the method parameters with the block parameters, but I hadn't considered the docs. I'll sort this out, thanks. 👍
urbanautomaton
force-pushed
the
deprecate-ab-seen-with-alternative-instance
branch
from
1378e9a
to
79c9fa9
Compare
Currently all methods in the storage adapter interface that accept an `alternative` argument accept it in the form of an alternative ID. The exception to this is `#ab_seen`, which accepts an Alternative instance. Since third-party code might depend on this, this change at first deprecates the old usage while allowing both calling styles. The old usage can then be removed in a future version.
In the previous commit, calling `#ab_seen` with an alternative instance was deprecated in favour of calling with an alternative id, to bring the method signature into line with the rest of the abstract adapter API. This updates the only internal caller to the new style.
urbanautomaton
force-pushed
the
deprecate-ab-seen-with-alternative-instance
branch
from
79c9fa9
to
469917a
Compare
|
Thanks for the feedback - I've restored the descriptive parameter names, and used short names for the inner block parameters to avoid shadowing but keep things descriptive. Please don't apologise for the review time - really appreciate you looking at this series of PRs lately. Hope you had a great holiday. :) |
As noted in #309, the
#ab_seenadapter method currently takes an instance ofAlternative, where all other adapter methods take an alternative id.Since this has the potential to cause confusion, I thought it might be best to support both calling styles, logging a deprecation notice if the method is called with an
Alternativeinstance, but still returning the correct response. We can then remove support for the old method signature after a respectful period of silence. :-)(I've also silenced the deprecation warnings emitted by another test, hope that's okay.)