`t:Hammer.key/0` is `term()`, but all backend expect `String.t()`

[Gladear]

Describe the bug

Hammer.key/0 is set to value term(), so we should be able to add any key. However, in all backends (e.g. ETS.FixWindows, Atomic.LeakyBucket, Redis.TokenBucket), @spec of implementations require the key to be a string. More importantly, while non-binary values do work in most backends, the Redis one actually require the key to be a string, and crashes otherwise.

** Provide the following details

• Version of the library: 7.0.1
• Backend used: *
• Algorithm used: *
• Elixir version (elixir -v): N/A
• Erlang version (erl -v): N/A
• Operating system: N/A

Expected behavior

Should the backend work with any term? Or should the key be updated to be a string? Having any term as key can be pretty useful, but does not seem to fit all backends.

I'm open to make the required changes in a PR, but I'd rather wait a decision on the matter beforehand 😄

[epinault]

thanks for reporting this! I am not sure if there is a need of non string key and I am open to it. I would prefer to make them term in most and just limit redis to be a string would be my direction. Which means it s still a term just restricted to string in redis only

[Gladear]

Hello, sorry for the delay. For what it worth, I'd rather have all backends use the same types of keys, so it's easier to change if necessary. But if you decide to make the change for some backends only, well, I can apply this change 😄

[epinault]

are you sayin you would prefer to limit to string only? or have the same time, and for redis figure out a way to serialize to string when it s not? that another option too

[Gladear]

Both options sound good to me! The "string-only" option might be a little less magic, but the "term-everywhere" option does not break compatibility.

[epinault]

yea, I ll fix it with term everywhere, and handle in redis to convert the key . And if not a string, I ll add something to the doc to warn about it

[epinault]

I have an MR almost ready to go