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

Drop Unidecode Dependency #1208

Closed
AlecRosenbaum opened this issue Jun 15, 2020 · 4 comments · Fixed by #1212
Closed

Drop Unidecode Dependency #1208

AlecRosenbaum opened this issue Jun 15, 2020 · 4 comments · Fixed by #1212
Assignees
Labels

Comments

@AlecRosenbaum
Copy link
Contributor

Related: #1079

PR #1080 (which fixes the above linked issue) added a new dependency to this project, Unidecode.

I'd like to suggest dropping this dependency, and possibly removing this utility function.

From what I can tell the to_const utility is not actually used in this library at all, and just from spot checking seems to only be used by graphene-django. The linked issue specifically uses a graphene-django example. If this is only used upstream in one spot, it might be worth moving the utility + dependency there instead.

It's possible the average use case also involves graphene-djagno and would require this dependency regardless, but for those that don't it would be nice not to require unidecode for a single-line utility function that may or may not be used.

While neither package is huge, unidecode is a few times larger than graphene according to pypi's downloads.

Depending on what the best option forward here is, I am happy to submit a PR that either:

  • removes the to_const function + drops unidecode
  • makes unidecode an optional dependency specified by
    extras_require: { 'unidecode': "unidecode>=1.1.1,<2", ... }
  • reverts to a simpler unicode decode handler, with something like str(string, errors="ignore") + drops unidecode
@jkimbo
Copy link
Member

jkimbo commented Jun 16, 2020

@AlecRosenbaum I think it's important that the functionality stays the same but I'm open to dropping the dependency. If you can create a PR to remove unidecode but keep the tests passing then I'll happily merge.

@AlecRosenbaum
Copy link
Contributor Author

I'm not sure it's possible to maintain this specific test case without large lookup tables, which is what unidecode is from what I can tell. But that's kind of the point I'm raising here -- it doesn't make sense to me to include large unicode lookup tables to make a one-line utility function only used downstream in graphene-django pass this test case.

def test_to_const_unicode():
assert to_const("Skoða þetta unicode stöff") == "SKODA_THETTA_UNICODE_STOFF"

It would be possible to make it not error and produce a name that's valid according to graphql.utils.assert_valid_name.assert_valid_name, but the output would of course change:

print(
  re.sub(
    r"[\W|^]+", "_",
    "Skoða þetta unicode stöff".encode('ascii', errors='ignore').decode('utf8')
  ).upper()
)
# 'SKOA_ETTA_UNICODE_STFF'

@jkimbo
Copy link
Member

jkimbo commented Jun 24, 2020

@AlecRosenbaum I see what you mean now and I agree, it makes sense to move the unidecode dependency downstream to the graphene-django repo.

@jkimbo
Copy link
Member

jkimbo commented Jun 24, 2020

Created a PR in Graphene-Django to move the to_const function: graphql-python/graphene-django#992 and a PR in Graphene to remove the unidecode dependency #1212

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