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

Add pyuca to the requirements #335

Closed
adamsol opened this issue Feb 23, 2021 · 2 comments · Fixed by #337
Closed

Add pyuca to the requirements #335

adamsol opened this issue Feb 23, 2021 · 2 comments · Fixed by #337
Labels

Comments

@adamsol
Copy link
Contributor

adamsol commented Feb 23, 2021

Currently, pyuca is listed in readme as an optional package "for more accurate sorting of translated country names". Unfortunately, this makes it quite difficult to find why pyuca is installed with an application if no comment has been left. I was ready to remove pyuca from my application's requirements, and only after some digging in the repository have I found that it's used here (and it matters for my language).

A better solution would be to install the package automatically with django-countries. This way it will show as required in pip show pyuca.

I guess that pyuca is not used by default due to performance reasons (#330). If this is the case, then a configuration variable (e.g. COUNTRIES_UNICODE_COLLATION, just like COUNTRIES_FIRST) would be a good solution.

@SmileyChris
Copy link
Owner

On the other hand, always having the package install when you don't need it though is a bit "bloaty". The correct solution is to add it as an optional extra, then recommend installing like pip install django-countries[pyuca] if required.

Care to throw that together as a pull request?

@adamsol
Copy link
Contributor Author

adamsol commented Feb 24, 2021

You're right, an optional dependency is probably a better idea if the package is not to be used in most cases. It seems that with this solution pyuca still won't be displayed as required by django-countries in pip show pyuca, but at least it will be clear in the requirements what it is used for.

I've created a PR: #337.

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

Successfully merging a pull request may close this issue.

2 participants