Add limits to #log_deprecation

[delner]

What does this PR do?

This adds a limiting feature to the log_deprecation function. By default, there is no limit. However, if you provide a key:, then it will prevent any other message logged with the same key thereafter.

Motivation:

Some deprecated behavior may be on a frequently used path that could produce a lot of noise. Instead of logging every time, or never logging at all, make it possible to warn once.

Additional Notes:

I extracted the existing log_deprecation function into its own module (Core::Deprecations) and recomposed it back into the original Core module. Now that the deprecation behavior was becoming more sophisticated, I didn't want the core.rb file to scope creep; thus I isolated the behavior to its own file and module.

How to test the change?

Run the test suite.

[lloeki]

• From Hash.new(0) the expected hash values are Integer.
• From _logged_deprecations[key] += 1 the hash values are indeed Integer and the keys are key.

To avoid repetition, let's call the key type "key" by defining a type alias.

Note: RBS type aliases are all lowercase, distinguishing them from Ruby types which are constants and thus start with an uppercase letter.

Note: the apparent key name clash is resolved at .rbs parse time, as there is no actual parsing ambiguity between type aliases and Ruby identifiers.

Also, from our discussion, the expected behaviour of key is to behave correctly as a hash key, which means it must respond to #hash and #eql?. Let's define an interface _Hashing for that.

Note: RBS interfaces start with an underscore followed by an uppercase letter. This isolates them from Ruby types, which follow constant conventions.

Note: _Hashing comes from the RBS doc.

Note: _*ing (_Hashing, _Reading, ...) is apparently a conventional way to name Ruby interfaces by "what they do", comparable to the *er convention in Go (e.g Stringer, Reader, Writer, ReadWriter, ReadWriteCloser, ...). I did not notice that and have been using a *able convention myself (_Stringable, _Readable, ...).

Note: since _Hashing might be used throughout we may ultimately want to hoist it in an upper level namespace and make more extensive and consistent use of it. Not in this PR though.

This gives us:

Suggested change

	@_logged_deprecations: untyped
	interface _Hashing
	def hash: () -> ::Integer
	def eql?: (untyped) -> bool
	end
	type key = _Hashing
	@_logged_deprecations: ::Hash[key, ::Integer]

[delner]

So IIUC, we don't do @logged_deprecations: ::Hash[::Object, ::Integer] because Object may not respond to the functions that make it "hashable", therefore, we define an interface to which objects used as Hash keys must conform to?

[lloeki]

Correct. In addition:

• a BasicObject child instance may actually be hashable
• an Object child instance may have their hash and eql? methods removed
• it gets precise and explicit as to what is actually required, both in naming (Object is unclear as to what's the requirement is) and in which methods (through the definition of the interface)

[TonyCTHsu]

I would like to see some changes to the method naming from this module. Currently, it is confusing because everything named with combination of log and deprecation and a bunch of !, ? _. Those knowledge should already been capture under the namespace of Deprecation.

[delner]

@TonyCTHsu

• everything named with combination of log and deprecation: this is because its a composable module: it could conflict with other method names in the class its composed into.
• a bunch of !, ? _: I agree that I can strike the underscore in the one name. But the ? and ! are Ruby convention for those methods.

[delner]

Thanks @lloeki for the detailed feedback on the RBS! I will try to apply that, and submit again for your consideration.

[p-datadog]

If I am understanding this PR correctly, each class/module that extends Deprecations will get its own tracker of what was already warned about. I think this would be useful to include in the documentation for Deprecations.

[delner]

@p-datadog Technically true, but its intention is to be composed into the Core module itself, so I don't really expect anyone to extend it.

[delner]

@lloeki the guide and explanation about how to properly RBS type this was great. I definitely learned some things about RBS. Thank you! I think this is ready for another look.

[lloeki]

Missed that one: probably this needs an update to sig/datadog/core.rbs with:

module Datadog
  module Core
  end

  extend Core::Configuration
  extend Tracing::Contrib::Extensions::Helpers
  extend Core::Deprecations
end

[lloeki]

each class/module that extends Deprecations will get its own tracker of what was already warned about. I think this would be useful to include in the documentation for Deprecations.

I don't really expect anyone to extend it.

It might be possible to enforce this via a self type constraint:

Module declaration takes optional self type parameter, which defines a constraint about a class when the module is mixed.

Something like (not quite sure):

Suggested change

	module Deprecations
	module Deprecations : Core

[delner]

Adding this with the change above causes:

sig/datadog/core.rbs:5:2: [error] Module self type constraint in type `::Datadog` doesn't satisfy: `singleton(::Datadog) <: ::Datadog::Core`
│ Diagnostic ID: RBS::ModuleSelfTypeError
│
└   extend Core::Deprecations
    ~~~~~~~~~~~~~~~~~~~~~~~~~

[datadog-datadog-prod-us1]

Static Analysis Violation (ruby-best-practices/concat-strings)

⚪ Notice - Best Practices - Link to Results

Avoid appending to string using +=

Suggested change

	message += ' This will be enforced in the next major release.' if disallowed_next_major
	message << ' This will be enforced in the next major release.' if disallowed_next_major

Leave feedback in #static-analysis