fix: use str() to trigger eventual django's gettext_lazy string

strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension

MIT License

391 stars 115 forks source link

fix: use str() to trigger eventual django's gettext_lazy string #493

Closed fabien-michel closed 3 months ago

fabien-michel commented 3 months ago

The generate enum from choices feature fail when choice description is a django's lazy string (gettext_lazy used with translation framework)

Description

Use str() on description to trigger string translation when generating Enum.

Types of Changes

[ ] Core
[x] Bugfix
[ ] New feature
[ ] Enhancement/optimization
[ ] Documentation

Issues Fixed or Closed by This PR

Fix #489

Checklist

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have read the CONTRIBUTING document.
[x] I have added tests to cover my changes.
[x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

sdobbelaere commented 3 months ago

@fabien-michel I also tried your suggestions on my end for my project and it seems to complain if there is for example a hyphen in the enum. Eg locales, our app is multilingual and this means we have stuff like "en-gb", "fr-fr".

Currently this is throwing errors that the characters are not [a-z,A-Z]. What do you think about this? Should that restriction be here?

fabien-michel commented 3 months ago

Thanks for your comment. This is not exactly the point of this PR, because it focus on handling django's lazy strings. But your statement is true, since choice's value can be any string (I suppose), it is not compatible with GraphQL requirement (it should validate this regex [_a-zA-Z0-9]). I suppose it may be the responsibility of the "generate from choices" helper to convert the choice value into a valid GraphQL name. If this is the case, is there a common/known way to convert string to valid GraphQL name?

bellini666 commented 3 months ago

@fabien-michel @sdobbelaere OMG I reviewed the PR and didn't notice there was a discussion going on.

Currently this is throwing errors that the characters are not [a-z,A-Z]. What do you think about this? Should that restriction be here?

This made me worry a bit.. was this solution not supposed to be merged? I'll avoid doing a new release until I understand this

fabien-michel commented 3 months ago

The bug already exists in present code. If you have a choice key having incompatible chars.

class Choice(models.TextChoices):
   EN_US = "en-US", "English"

GraphQL will raise an Exception because the "-" is not allowed as enum value. I'm working on a solution by replacing all incompatible characters by "_".

sdobbelaere commented 3 months ago

@bellini666 Sorry, didn't mean to interrupt this PR. As @fabien-michel says, it seems to be thrown by the Graphql package, not strawberry.

bellini666 commented 3 months ago

Oh I see. Thanks for the explanation.

I also saw another PR fixing the issue, will review it :)