oculus42 / short-uuid

Translate standard UUIDs into shorter formats and back.
MIT License
450 stars 13 forks source link

Behavior of Prevent Repeated Characters #48

Closed oculus42 closed 3 years ago

oculus42 commented 3 years ago

Brought up in #47 the idea of stripping repeated characters from an alphabet is simple, but I'm not sure about the appropriate way to implement. Mentioned in that thread:

Unlike the padding, it creates a condition where two instances may translate something differently. I would be more inclined to have the library throw an error if you try to use an alphabet with repeating characters without setting an explicit repeat flag rather than have it silently change the alphabet.

thadeucity commented 3 years ago

I altered the code as followed, can I send a pull request?

        // Check alphabet for duplicate entries
        if ([...new Set(Array.from(useAlphabet))].length !== useAlphabet.length) {
            throw new Error('The provided Alphabet has duplicate characters resulting in unreliable results');
        }
thadeucity commented 3 years ago

Thank you! That was a nice milestone for me. First pull request for an open project. I started programming at the beginning of the year, I'm glad my contribution was useful.

wparad commented 3 years ago

Huh? Don't throw an error when you can just stripe them out. There's never a good reason to have duplicated characters.

A better solution is to contain a full character list and use the input as a filter, that way the order will always be preserved, and accidents like changing character order (which has no semantic meaning) won't cause a problem for users.

Obviously that requires a new non-backwards compatible version of the library, but if we want to be safe AND provide a good experience that's the way to go.

oculus42 commented 3 years ago

You are correct that there is never a good reason to have duplicated characters, which is why throwing an error is the appropriate behavior. Duplicated characters are bad input. The consumer of the library should be aware they have a problem with their alphabet rather than the library quietly changing the alphabet.

oculus42 commented 3 years ago

No further discussion so closing. Please re-open if you feel it is warranted.