Skip to content

Enum conversion fixes #665

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.

Sign up for GitHub

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

Closed
wants to merge 14 commits into from
Closed

Enum conversion fixes #665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3f35b1f
cover enum case where choice begins with a number
zbyte64 May 30, 2019
8d42c7c
chomp out non-ascii characters from enum name generation
zbyte64 May 31, 2019
1bcc1ef
fix python 2 test regression
zbyte64 May 31, 2019
5b45c5d
fix python 2 test regression: set source file encoding to utf8
zbyte64 May 31, 2019
e3017b5
increment version to match release tag
mvanlonden May 31, 2019
9275163
test and handle choices that start with an underscore, issue #141
zbyte64 Jun 5, 2019
71ba7fb
Merge remote-tracking branch 'up/master' into enum_conversion_fixes
zbyte64 Jun 10, 2019
6008e4d
fix unit tests
zbyte64 Jun 10, 2019
43233d7
expand test assertions so that test enum conversion with underscores …
zbyte64 Jun 10, 2019
59184a1
prefix underscore choice name only if it is dunder, otherwise undersc…
zbyte64 Jun 11, 2019
de7edaa
expand test case to cover underscores that are sunder, dunder, and ge…
zbyte64 Jun 11, 2019
4ba3b54
Merge remote-tracking branch 'up/master' into enum_conversion_fixes
zbyte64 Jun 17, 2019
6d5cfee
apply black formatting
zbyte64 Jun 17, 2019
8f354ee
cleanup test choices
zbyte64 Jul 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion graphene_django/converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,31 @@
singledispatch = import_single_dispatch()


def _is_dunder(name):
"""Returns True if a __dunder__ name, False otherwise."""
return (
len(name) > 4
and name[:2] == name[-2:] == "__"
and name[2:3] != "_"
and name[-3:-2] != "_"
)


def _is_sunder(name):
"""Returns True if a _sunder_ name, False otherwise."""
return (
len(name) > 2
and name[0] == name[-1] == "_"
and name[1:2] != "_"
and name[-2:-1] != "_"
)


def convert_choice_name(name):
name = to_const(force_text(name))
name = force_text(name).encode("utf8").decode("ascii", "ignore")
name = to_const(name)
if _is_sunder(name) or _is_dunder(name):
name = "A%s" % name
Copy link
Member

Choose a reason for hiding this comment

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

@zbyte64 I'm a bit confused with what is happening here. If the name starts with _ you're prefixing it with a A? Why is that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

assert_valid_name requires the name not to start with an _. Without this, a name like _amount would be caught and A_ would be prefixed to become A__amount. With this change, the name becomes A_amount, removing the extra _

Copy link
Member

Choose a reason for hiding this comment

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

@zbyte64 I don't think that is right. Running assert_valid_name on a values with both _ and __ work fine:

In [1]: from graphql import assert_valid_name

In [2]: assert_valid_name('foo')

In [3]: assert_valid_name('_foo')

In [4]: assert_valid_name('__foo')

Copy link
Collaborator Author

@zbyte64 zbyte64 Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Erm, I mispoke. assert_valid_name will accept names that start in _ but graphene's Enum will skip over them when populating members. Thus we need to prefix this special case ourselves.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Specifically in the code where this happens: https://github.com/graphql-python/graphene/blob/6c7cd4e4fa6e29f55ebf02f173281d46d9f8fc1c/graphene/pyutils/enum.py#L109

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I see now. I expanded the test case to cover the various underscore mixtures.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does a normal Python enum handle these sunder and dunder values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Creating a python enum with a sunder causes:
ValueError: _names_ are reserved for future Enum use

Dunder is allowed but is ignored (doesn't generate a member).

Copy link
Contributor

Choose a reason for hiding this comment

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

Then does it make sense to have the same limitations in the Graphene Enum type instead of tacking A_ on the front?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're already converting choices that wouldn't be allowed, like those that start with a number.

try:
assert_valid_name(name)
except AssertionError:
Expand Down
9 changes: 8 additions & 1 deletion graphene_django/tests/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
# -*- coding: utf-8 -*-
from __future__ import absolute_import

from django.db import models
from django.utils.translation import ugettext_lazy as _

CHOICES = ((1, "this"), (2, _("that")))
CHOICES = (
(1, u"this"),
("2", _(u"that")),
(u"_3漢", "nonascii"),
("__dunder__", "dunder"),
("_sunder_", "sunder"),
)


class Pet(models.Model):
Expand Down
21 changes: 21 additions & 0 deletions graphene_django/tests/test_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,27 @@ class Meta:
convert_django_field_with_choices(field)


def test_field_with_choices_underscore():
field = models.CharField(
choices=(
("_amount_", "Amount"),
("__percentage__", "Percentage"),
("_not_sunder__", "Not Single Underscore"),
("__not_dunder", "Not Double Underscore"),
)
)

class UnderscoreChoicesModel(models.Model):
ourfield = field

graphene_type = convert_django_field_with_choices(field)
assert len(graphene_type._meta.enum.__members__) == 4
assert "A_AMOUNT_" in graphene_type._meta.enum.__members__
assert "A__PERCENTAGE__" in graphene_type._meta.enum.__members__
assert "_NOT_SUNDER__" in graphene_type._meta.enum.__members__
assert "__NOT_DUNDER" in graphene_type._meta.enum.__members__


def test_field_with_choices_convert_enum_false():
field = models.CharField(
help_text="Language", choices=(("es", "Spanish"), ("en", "English"))
Expand Down
3 changes: 3 additions & 0 deletions graphene_django/tests/test_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,9 @@ def test_schema_representation():
enum ReporterAChoice {
A_1
A_2
_3
A__DUNDER__
A_SUNDER_
}

enum ReporterReporterType {
Expand Down