Remove lazy option from the configuration option

[GustavoCaso]

What does this PR do?

This PR removes the lazy option in favour of checking the content of default.

These changes are part of an internal discussion of the ruby team 😄

Motivation

The lazy option is superfluous, the check to either evaluate default as a block or a static value could be achieved by checking the type of value default is. default.is_a?(Proc)

There is an edge case:
The default value is a block.

When we provide a block to default and do not specify it as lazy, we store the block as the default value. I added a new option, experimental_default_proc, to circumvent that edge case. Pushing us to be more explicit about those edge cases makes the code more readable and easier to grok.

Since this new option is experimental and we might not need it in the near future, I gave it a name that it clear to the reader that they should not rely on it and that is subject to change.

I added a deprecation warning to lazy, so if anyone is using the core code to build their settings, they can migrate to the new system.

Additional Notes

How to test the change?

CI

[codecov-commenter]

Codecov Report

Merging #2931 (569713b) into master (0e396e9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #2931    +/-   ##
========================================
  Coverage   97.95%   97.95%            
========================================
  Files        1282     1282            
  Lines       70682    70438   -244     
  Branches     3250     3247     -3     
========================================
- Hits        69236    68997   -239     
+ Misses       1446     1441     -5     

Impacted Files	Coverage Δ
lib/datadog/ci/configuration/settings.rb	100.00% <ø> (ø)
lib/datadog/core/configuration/base.rb	97.95% <ø> (-0.05%)	⬇️
...ib/datadog/core/configuration/option_definition.rb	100.00% <ø> (ø)
lib/datadog/tracing/configuration/settings.rb	94.11% <ø> (-0.93%)	⬇️
...ing/contrib/action_cable/configuration/settings.rb	100.00% <ø> (ø)
...ng/contrib/action_mailer/configuration/settings.rb	100.00% <ø> (ø)
...cing/contrib/action_pack/configuration/settings.rb	95.45% <ø> (-0.55%)	⬇️
...cing/contrib/action_view/configuration/settings.rb	100.00% <ø> (ø)
...active_model_serializers/configuration/settings.rb	100.00% <ø> (ø)
...ng/contrib/active_record/configuration/settings.rb	100.00% <ø> (ø)
... and 48 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[marcotc]

👋 @GustavoCaso, why is lazy being removed?

[GustavoCaso]

@marcotc I am preparing an RFC to reduce some of the complexity of our core configuration code and options. By any means, this is something we have to do, I instead want the guild to agree, but for the cases of the lazy option, I wanted to have an example PR for everyone to have a look at.

[lloeki]

In my mind lazy referred to when a proc (or callable, really) is evaluated:

• default may be a callable
• some settings may allow callables to be set as values (IIRC there's transport)

lazy decides whether these are evaluated upon the setting being set or upon the setting being called once.

So:

• I don't really see a solid argument for a callable being evaluated upon being set (except maybe if there are settings that should store a callable that is not meant to be called to resolve to the setting value i.e if the callable is itself the value)
• if a non-callable is set then by definition the value is evaluated right away

If we hold the above as true then it looks like lazy is indeed superfluous as we can check for respond_to?(:call).

[ivoanjo]

Overall I really like this diff!

I think the missing bit is being a bit more clear on our advice to customers (do we want them to use experimental_default_proc? And are they aware it may disappear soon?).

This is one situation where the warnings, as well as the PR description should be considered with advanced customers in mind as the readers, not only our team.

[ivoanjo]

Two notes on this:

• "You no longer need to" should perhaps be "you no longer can" -- this option is gone, it's not that you wouldn't ever need it
• Is it worth telling people to use the experimental option? Should we instead tell them that we don't support it all until we figure out what we should do?

[ivoanjo]

👍 LGTM. It may be worth to triple-check that everyone is happy with this internally, but I see no reason not to merge this in :)