Skip to content
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

Allow empty string prefix separator #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

taylordowns2000
Copy link

@taylordowns2000 taylordowns2000 commented Dec 13, 2020

Not sure if this has come up before, but I'd love to not use a prefix separator... does this do the trick? Really cool implementation of hashids, btw. Thank you.

@coladarci
Copy link
Collaborator

Glad you find this useful! The only real thing wrong with the library (which I haven’t had time to think through yet) is the annoying requirement to manually recompile the dep when you add/change your desired config - curious if you ran into this and if you have any suggestions on that front...

@coladarci
Copy link
Collaborator

I have to look into your solution more closely - if there isn’t a separator we’d have to make assumptions that the module identifier is always single character? I’m not against this it just might bite you down the line... thoughts?

@taylordowns2000
Copy link
Author

Ah! Yes—ran into it but I don't have too much experience writing Elixir libraries so steered clear. My gut instinct was that using @salt, @prefix_separator, etc. was unnecessary and forced the manual recompile. Normally I'd be fine with calling Application.get_env(...) inside line 35, for example.... but I suppose that wouldn't work here since you actually have to compile a module using those module attribute unary operators.

On multi-character identifiers... I had not thought of that; definitely could be a gotcha. How does Module naming work with that currently, with respect to capitalisation? I must admit I was a little confused about how a prefix r became a module R, and how it doesn't seem that capital letters are allowed in the Hashids themselves.

@coladarci
Copy link
Collaborator

coladarci commented Dec 13, 2020

The idea is that you have your domain model - MyApp.User - let’s say you want to have the ID not be sequential; you’d choose a prefix for it; probably “u”. So if there wasn’t a separator you’d have an ID like u[hash]. So as long as the library can assume if separator is “”, the first letter is always the module. This is fine for small apps, but the moment you have 30 models in your app, you’ll regret the decision.

What’s the reason for not wanting a separator? If it’s aesthetic, you might just strip the separator when displaying it?

@taylordowns2000
Copy link
Author

taylordowns2000 commented Dec 13, 2020 via email

@coladarci
Copy link
Collaborator

coladarci commented Dec 13, 2020 via email

@taylordowns2000
Copy link
Author

taylordowns2000 commented Dec 14, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants