-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Does the bucket name have to be a string? #17
Comments
Sorry for the delay. I have to state up-front, I'm a little rusty on this since I'm not currently using Elixir/Erlang. However, I think what you are doing may work, but I'm not sure its recommended. Here is an example of where that is being called: https://github.com/grempe/ex_rated/blob/master/lib/ex_rated.ex#L215 and the Key is already of the form Looking at ETS docs: https://elixirschool.com/lessons/specifics/ets/ It looks like the Key can be an Erlang So it may work, but I'm not sure if that is accidental or not. If you want to dig into this some more and submit a PR updating docs and tests to accept that I would certainly take a look. I'm not sure what the benefits are though of doing it this way. Maybe you can elaborate. |
If it's just passing it on to ETS then it's certainly allowed. The benefits are many; in my case I'm rate limiting IPs from Phoenix's |
OK, if you want to work up a docs change and add tests to demonstrate I'll be happy to take a look. |
Alright. One other question: are buckets unique by their scale and limit, or just by their name? |
Well the key is tangled with the scale to some degree in the following function that creates the key, but I would not say the bucket is unique across all those values. https://github.com/grempe/ex_rated/blob/master/lib/ex_rated.ex#L254 |
I'll mark this as help wanted. If someone would like to add tests including the use of any Erlang |
I'm using tuples for bucket names and they seem to work fine, however the README and typespecs explicitly say string. I briefly looked through the code and didn't see anything that required bucket names to be strings, but I may have missed something.
If they're not restricted to strings it might be helpful to update the docs and typespecs.
The text was updated successfully, but these errors were encountered: