search

sqids / sqids-go

Official Go port of Sqids. Generate short unique IDs from numbers.
https://sqids.org/go
MIT License
536 stars 11 forks source link

`Options` are no longer pointers, variadic `New` #10

Closed peterhellberg closed 1 year ago

peterhellberg commented 1 year ago

🔥 Removed no longer needed NewCustom and check for inRangeNumbers in Encode (since a []uint64 can only contain 0 or more uint64 values, all of which are valid to encode)

Updated examples i README, should better align with JavaScript implementation now that we do not have to have a separate NewCustom constructor.

I'm also introducing verifiable code in the README (using embedmd) in order to more easily keep the examples up to date.

Changes

4kimov commented 1 year ago

@peterhellberg MinValue/MaxValue are part of the spec as convenience methods, so users can have programmatic access (some langs might have dynamic max value). Do we really need to remove them?

peterhellberg commented 1 year ago

@peterhellberg MinValue/MaxValue are part of the spec as convenience methods, so users can have programmatic access (some langs might have dynamic max value). Do we really need to remove them?

I have a fairly hard time in seeing the utility of an exported function returning 0 for the minimum value of a unsigned integer, would it in any implementation be anything else than 0? And (in typed languages at least) there is no need to check if an unsigned integer is "in range", maybe I'm missing some context about why this would be needed in the first place 🤔

I'm personally quite hesitant to have something that might be used at some point in the future in a public API (when there are no clear indications that his would be the case as of right now)

4kimov commented 1 year ago

This would've been an ideal discussion for here and here, when I was looking for active feedback regarding the spec.

Now that the spec is solidified, all implementations are being uniform in their API and output. I think the proper way to go about this is 1) keep it as is for now on the per-repo basis (including this one) and 2) create an issue in the spec repo advocating for either getting rid of both (MinValue/MaxValue) or just one (MinValue), and let everyone chime in given enough time - so that when it does pass, this can propagate properly down thru all individual repos.

P.S: The min value function was proposed initially because in the Hashids days, I'd constantly get either questions on encoding negative numbers or feature requests to add support.

peterhellberg commented 1 year ago

Ok, and you are fine with sqids.MinValue() and sqids.MaxValue() not being used in the Go library? (Since there is no need to check that a []uint64 contains 0 or more uint64 values in Go)

I'll update the PR to retain the MinValue/MaxValue functions.

4kimov commented 1 year ago

Yes, the "in range" check that you mention earlier was an oversight that was accidentally transferred over from other libs, and we don't have to use those min/max functions within the library itself if there's no need.

I'll update the PR to retain the MinValue/MaxValue functions.

Sounds good, thank you.

peterhellberg commented 1 year ago

@4kimov I've rebased this PR on top of the latest main, do you think these changes are good to be merged or would you like some additional changes before doing so?

4kimov commented 1 year ago

@peterhellberg Looks very nice and cleaned up. Thank you for all the changes. I'm good with merging.

Btw, I've been keeping a CHANGELOG.md, should we remove it altogether or update it with newest changes?

peterhellberg commented 1 year ago

@peterhellberg Looks very nice and cleaned up. Thank you for all the changes. I'm good with merging.

Cool, will do.

Btw, I've been keeping a CHANGELOG.md, should we remove it altogether or update it with newest changes?

I think we can remove the CHANGELOG.md and instead create GitHub releases for the tags if there is a need to describe the changes from the previous release.