Enum conversion fixes #665
Conversation
zbyte64
requested review from
syrusakbary,
BossGrand,
danpalmer,
Eraldo,
firaskafri,
patrick91,
phalt,
spockNinja and
jkimbo
| name = force_text(name).encode('utf8').decode('ascii', 'ignore') | ||
| name = to_const(name) | ||
| if name.startswith('_'): | ||
| name = "A%s" % name |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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 _
There was a problem hiding this comment.
@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')There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Specifically in the code where this happens: https://github.com/graphql-python/graphene/blob/6c7cd4e4fa6e29f55ebf02f173281d46d9f8fc1c/graphene/pyutils/enum.py#L109
There was a problem hiding this comment.
I think I see now. I expanded the test case to cover the various underscore mixtures.
There was a problem hiding this comment.
How does a normal Python enum handle these sunder and dunder values?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Then does it make sense to have the same limitations in the Graphene Enum type instead of tacking A_ on the front?
There was a problem hiding this comment.
We're already converting choices that wouldn't be allowed, like those that start with a number.
…is more interpretable
…ores like sunder are allowed
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Collected backwards compatible fixes from #654
Fixes #141 and allows using non-ascii characters as choice values.