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

feat: Allow compatibility with other ShortUUID libraries (#104) #107

Merged
merged 4 commits into from
Dec 10, 2024

Conversation

Thiagomrfs
Copy link
Contributor

My attempt to fix: #104.

I've added should_sort kwarg to set_alphabet, whenever this is set to False, the code will use an ordered set rather than the default behaviour of sorting an unordered set.

Other than that, I've tried to integrate this behaviour into the Django Field. I don't have any experience working with custom Django fields, so I'd appreciate if someone could review commit 077ed4.

@skorokithakis
Copy link
Owner

This is getting the causality wrong, as this is the original ShortUUID library, which means that the other libraries aren't compatible with this one. I'm not sure whether this library needs to change because someone copied it and made it incompatible, but maybe it won't hurt to be practical. I'll review the PR now and see, thank you for the contribution!

@skorokithakis
Copy link
Owner

This looks ok, I guess it can't hurt anything. However, could you rename the argument to dont_sort_alphabet and make it default False? Also, could you add some documentation to explain what the argument does, and why it's required?

Thanks again!

@Thiagomrfs
Copy link
Contributor Author

Thanks for the response, I'll get at it shortly.

@Thiagomrfs
Copy link
Contributor Author

Just finished the requested changes!

@skorokithakis
Copy link
Owner

Looks good, thank you! Do you know whether libraries with repeated characters work? They should, since you're using a dict, but I just want to make sure.

Copy link
Owner

@skorokithakis skorokithakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thank you!

@skorokithakis skorokithakis changed the title fix: Sorting the alphabet causes incompatability with other shortuuid libraries (#104) feat: Allow compatibility with other ShortUUID libraries (#104) Dec 10, 2024
@skorokithakis skorokithakis merged commit 6843c12 into skorokithakis:master Dec 10, 2024
@Thiagomrfs
Copy link
Contributor Author

Sorry, I didn't quite understand your question.

You meant to ask if libraries with non-unique characters in the alphabet would work?

Since dictionaries preserve insertion order, but can't have duplicated keys, I used it to eliminate duplicates while maintaining the original order of the characters in the alphabet string.

I mainly use this library when working with shortUUIDs, so I don't really have a grasp of other libraries. Maybe the issue's OP could answer that.

@skorokithakis
Copy link
Owner

I was asking if character deduplication works as required or if, for example, it leaves the last character it sees, so 1a1 would become a1 instead of 1a. I'm sure it's fine, but also it removes compatibility with any Python before 3.7, which is probably OK.

@Thiagomrfs
Copy link
Contributor Author

Understood. It stores the first appearance of the character.

Using your example:

>>> string = "1a1"
>>> list(dict.fromkeys(string))
['1',  'a']

and

>>> string = "a1a1"
>>> list(dict.fromkeys(string))
['a', '1']

@skorokithakis
Copy link
Owner

Excellent, thank you!

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.

Sorting the alphabet causes incompatability with other shortuuid libraries
2 participants