Skip to content

Conversation

frensjan
Copy link

@frensjan frensjan commented Jul 7, 2017

When using django_enumfields field.choices is a list of the enum 'constants' and their labels / names. convert_choice_name renders the 'values' with str. This ok for normal choice values, but the string representation of python Enums follows the TypeName.VALUE format. This results in TYPENAME_VALUE formatted values for GraphQL queries, instead of VALUE (which I think it should be). This PR fixes that.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 91.558% when pulling 14a5d62 on TargetHolding:master into 0588f89 on graphql-python:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 91.558% when pulling 14a5d62 on TargetHolding:master into 0588f89 on graphql-python:master.

@coveralls
Copy link

coveralls commented Jul 10, 2017

Coverage Status

Coverage increased (+0.009%) to 92.729% when pulling 04c4d29 on TargetHolding:master into 0588f89 on graphql-python:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 92.729% when pulling 04c4d29 on TargetHolding:master into 0588f89 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 92.729% when pulling 04c4d29 on TargetHolding:master into 0588f89 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 92.729% when pulling 04c4d29 on TargetHolding:master into 0588f89 on graphql-python:master.

@Thibaut-Fatus
Copy link

this would be nice to have, and it fixes #67

@syrusakbary syrusakbary force-pushed the master branch 2 times, most recently from bdc7189 to f93251b Compare September 1, 2017 08:12
@syrusakbary
Copy link
Member

Like it! A test case for this scenario would be helpful for assuring that we don't break support in the future :)

@frensjan
Copy link
Author

I've joined a new company and am not working on GraphQL nor on python anymore. Should I close the PR?

@mvanlonden
Copy link
Member

While I think this would be nice to have, I'm worried about the dependence on django-enumfields. Comments like this lead me to worry hzdg/django-enumfields#96

Copy link
Collaborator

@danpalmer danpalmer left a comment

Choose a reason for hiding this comment

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

I think this PR, and the potentially related but possibly tangential #67, are indicative of a wider problem here. Issues I can see:

  • Is there too much "magic" going on with enum conversion? Maybe not.
  • Should choice fields really be represented as Enums?
  • Should this be opt-in?
  • Should the enum serialisation be clearer?
  • Should the documentation around how to customise Django model/form field conversion be clearer?

I feel like this particular change is a patch fix that isn't really understandable why it fixes things. Tests, comments, documentation and a more generalised solution are one option, and this can become a feature we support in some way.

Alternatively, improving the behaviour we have, making it more understandable, more customisable, and easier to modify, could also be a better approach.

@phalt
Copy link
Contributor

phalt commented May 3, 2019

I'd like to tidy up some of the PRs, so if we still want to work on this let me know or I will close this in 1 week.

@GitRon
Copy link
Contributor

GitRon commented May 8, 2019

I think the topic is still relevant though I'm not sure about the current PR.

I faced the problem that the default (and not changable) behavoir is returning really weird things from ChoiceFields. For example "A_1" instead of the 1 is would expect.

I monkey-patched the behavior so I could work with it but a configurable solution would be awesome.

@phalt
Copy link
Contributor

phalt commented May 8, 2019

@GitRon good point, as this is discussed heavily in other issues I will close this as this PR alone doesn't cover all the issues.

@phalt phalt closed this May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants