fix: forcefully disabling metrics when client is disabled (#139)

[rarruda]

About the changes

Disabling the client is meant for when a user will be testing their code, and no connection to a remote server is desired.

It is hard to see a use case for collecting and reporting metrics if the client is disabled. Forcefully disabling metrics when disabling the client makes sense and is a more coherent behavior.

Closes #139

Note that this change does change behavior, but to make things more sane.

Important files

• client.rb
• client_spec.rb

Discussion points

Also added WebMock.reset! in the client_spec.rb to ensure no leakage between tests.

Maybe it would have made more sense to have it overridden in lib/unleash/configuration.rb (by moving disable_metrics from attr_accessor to attr_writter + own getter) ?

[coveralls]

Pull Request Test Coverage Report for Build 4861812555

• 21 of 21 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 96.585%

---

Totals
Change from base Build 4526417167:	0.1%
Covered Lines:	2178
Relevant Lines:	2255

---

💛 - Coveralls

[thomasheartman]

Yeah, this looks pretty good to me! 😄 Some minor adjustment suggestions that you can choose to accept or reject.

Regarding the comment in the description:

Note that this change does change behavior, but to make things more sane.

Would you classify it as a bugfix? If not: is it a breaking change? From what I can tell, the previous behavior didn't really work and probably wasn't intended?

[rarruda]

Those were very good suggestions!

I can hardly see how the previous behavior would be desired, except in hyper-corner cases. (like when testing multiple times calling is_enabled?() / get_variant() for a feature, and checking that it has been invoked later on, and that the count is correct. But the user would have had to stub themselves the metric endpoint). I hardly see anyone using it this way. So I'd say it is a fix, but I can't say for sure that one has relied on previous behavior that would have changed either.

Makes sense?

But if i were to vote, I'd call it a fix.

[sighphyre]

Nice! LGTM