Consider a callback only module (behaviour definition) as all redudant

DianaOlympos commented 3 years ago

Bug Description

Hank consider that all these callbacks are useless because "they are not used anywhere in the module". Which is true. They are all used in other modules. This is a pure definition of a behaviour.

===> The following pieces of code are dead and should be removed:
src/hyper_register.erl:66: Callback bytes/1 is not used anywhere in the module
src/hyper_register.erl:63: Callback decode_registers/2 is not used anywhere in the module
src/hyper_register.erl:57: Callback encode_registers/1 is not used anywhere in the module
src/hyper_register.erl:53: Callback zero_count/1 is not used anywhere in the module
src/hyper_register.erl:49: Callback register_histogram/1 is not used anywhere in the module
src/hyper_register.erl:45: Callback register_sum/1 is not used anywhere in the module
src/hyper_register.erl:39: Callback reduce_precision/2 is not used anywhere in the module
src/hyper_register.erl:33: Callback max_merge/2 is not used anywhere in the module
src/hyper_register.erl:27: Callback max_merge/1 is not used anywhere in the module
src/hyper_register.erl:20: Callback compact/1 is not used anywhere in the module
src/hyper_register.erl:15: Callback new/1 is not used anywhere in the module
src/hyper_register.erl:7: Callback set/3 is not used anywhere in the module

To Reproduce

rebar3 hank in hyper

Expected Behavior

Show nothing...

Additional Context

OS: Linux rebar 3.16.1 on Erlang/OTP 24 Erts 12.0.3

elbrujohalcon commented 3 years ago

Hi @DianaOlympos 👋🏻

First of all, the rule is not talking about callback implementation, it's talking about callback usage (i.e. the piece of code where you write Module:the_callback(…)). Then, as it is stated in the rule docs…

For this rule to apply, it's assumed that callbacks defined for a particular behavior are only used within the same module that defines it. If you define behaviors in your project and you use their callbacks from other modules, you can add an ignore rule in rebar.config for it.

In other words, this rule assumes that you implement your non-OTP behaviors following these guidelines. If you don't abide by them, you should add an exception for this rule either by explicitly stating all the other rules in the rules subsection of the hank section of your rebar.config or by adding only this one to the ignore subsection, as explained here.

paulo-ferraz-oliveira commented 3 years ago

@DianaOlympos, @elbrujohalcon can probably confirm, but this is kind of similar to what Elvis already expects: that you implement your behaviour's usage in your behaviour file. I'm sure there's different use cases for this, but, at least for me, it's made my code much clearer.

DianaOlympos commented 3 years ago

I mean in this case, i cannot because the behaviour is basically just an interface for multiple backend for a data structure. The behaviour is the abstract data type for the different implementations.

elbrujohalcon commented 3 years ago

@DianaOlympos Well… that's exactly the situation I was facing when I wrote the articles. That's the source of the following tip…

If you really only need to define a behavior to be sure that all your types provide similar interfaces, don’t be afraid to add functions like the one below to your generic modules:
-module(figure).

-type t() :: #{module := module(), _ => _}.

-callback area(t()) -> float().

-export([area/1]).

-spec area(t()) -> float().

area(Figure = #{module := Module}) ->
    Module:area(Figure).
As you can see, the function area/1 is just a middleman. Its only goal is to allow other modules to evaluate figure:area(…) with any figure created in a callback module that implements the figure behavior. The alternative would be to force callers to figure out the callback module and evaluate the dynamic call themselves.

elbrujohalcon commented 3 years ago

This is how I would do it in your case: GameAnalytics/hyper#20

Either that or just move all the callbacks to hyper and just use -behavior(hyper). where you used -behavior(hyper_register). before.

Or move the logic from hyper to hyper_registerand leave hyper just as a facade module that calls hyper_register for the implementation.

DianaOlympos commented 3 years ago

I see. I need to modify that interface a lot anyway and for reference this is moving to the LivewareProblems as the source of truth cause the old GameAnalytucs one is not maintained anymore.

Thanks for the feedback!

AdRoll / rebar3_hank