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

fabien-michel · 2024-03-04T16:46:10Z

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

Issues Fixed or Closed by This PR

Fix #489

Checklist

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.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

sourcery-ai

PR Type: Bug fix

PR Summary: This pull request addresses a specific issue where the generation of enums from choices fails when the choice description is a Django's lazy string. The proposed solution involves using str() on the description to trigger string translation when generating Enums. This change is primarily a bug fix but also includes modifications to the testing suite to ensure the fix works as intended.

Decision: Comment

📝 Type: 'Bug fix' - not supported yet.

Sourcery currently only approves 'Typo fix' PRs.

✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.

No details provided.

✅ Small diff: the diff is small enough to approve with confidence.

No details provided.

General suggestions:

Consider adding more explicit test cases that cover different scenarios and edge cases for the translation of lazy strings in enums. This could help in ensuring that the solution is robust and works across various configurations.
Review the removal of explicit casting in strawberry_django/fields/types.py to ensure it does not introduce type mismatches or affect type safety negatively.
Ensure that the changes made to handle gettext_lazy strings are thoroughly documented, especially since this involves interaction with Django's internationalization framework. Clear documentation can aid in future maintenance and understanding of this fix.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

sourcery-ai · 2024-03-04T16:47:04Z

strawberry_django/fields/types.py

@@ -445,7 +444,6 @@ def resolve_model_field_type(
            int,
        )  # Exclude IntegerChoices
    ):
-        choices = cast(List[Tuple[Any, str]], model_field.choices)


suggestion (llm): Removing the explicit casting of model_field.choices to List[Tuple[Any, str]] simplifies the code. However, ensure that this change does not impact the type safety or introduce potential type mismatches elsewhere in the code, especially since the casting was likely introduced for a reason.

sourcery-ai · 2024-03-04T16:47:04Z

tests/test_enums.py

@@ -18,15 +19,15 @@ class Choice(models.TextChoices):

    A = "a", "A description"
    B = "b", "B description"
-    C = "c", "C description"
+    C = "c", gettext_lazy("C description")


suggestion (llm): It's great to see the addition of gettext_lazy for testing the translation of enum descriptions. However, to ensure comprehensive coverage, it would be beneficial to add explicit test cases that verify the translation is correctly applied and rendered for these enums. This could involve setting a different language temporarily in the test and asserting the translated string is as expected.

sourcery-ai · 2024-03-04T16:47:04Z

tests/test_enums.py



 class IntegerChoice(models.IntegerChoices):
    """IntegerChoice description."""

    X = 1, "1 description"
    Y = 2, "2 description"
-    Z = 3, "3 description"
+    Z = 3, gettext_lazy("3 description")


suggestion (llm): Similar to the previous comment on text choice enums, adding explicit tests for integer choice enums with gettext_lazy would ensure the translation mechanism works as expected across different types of enums.

sourcery-ai · 2024-03-04T16:47:05Z

tests/test_enums.py

@@ -36,13 +37,13 @@
        max_length=255,
        choices=[
            ("c", "C description"),
-            ("d", "D description"),
+            ("d", gettext_lazy("D description")),


suggestion (llm): For the changes in model choices using gettext_lazy, it's important to verify through tests that these changes do not affect the expected behavior of the model fields. Consider adding tests that create instances of these models and assert that the field values and descriptions are correctly handled and translated.

sourcery-ai · 2024-03-04T16:47:05Z

strawberry_django/fields/types.py

@@ -460,8 +458,9 @@
                        ),
                    ),
                    {
-                        c[0]: EnumValueDefinition(value=c[0], description=c[1])
-                        for c in choices
+                        # use str() to trigger eventual django's gettext_lazy string


suggestion (llm): The fix in the code to use str() for handling gettext_lazy strings is crucial. To ensure this change does not introduce any regressions, it would be beneficial to add unit tests specifically targeting this behavior. These tests could mock gettext_lazy to return different values based on the active language and assert that the enum descriptions are correctly translated.

sdobbelaere · 2024-03-04T19:14:39Z

@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 · 2024-03-04T21:08:38Z

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

Ty! :)

bellini666 · 2024-03-04T22:39:10Z

@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 · 2024-03-05T06:52:11Z

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 · 2024-03-05T09:19:42Z

@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 · 2024-03-05T13:45:41Z

Oh I see. Thanks for the explanation.

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

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

a286504

sourcery-ai bot reviewed Mar 4, 2024

View reviewed changes

bellini666 approved these changes Mar 4, 2024

View reviewed changes

bellini666 merged commit fc23726 into strawberry-graphql:main Mar 4, 2024
50 checks passed

fabien-michel mentioned this pull request Mar 5, 2024

Fix/489 auto enum value allowed chars #494

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fabien-michel commented Mar 4, 2024 •

edited by bellini666

Loading

sourcery-ai bot left a comment

sourcery-ai bot Mar 4, 2024

sourcery-ai bot Mar 4, 2024

sourcery-ai bot Mar 4, 2024

sourcery-ai bot Mar 4, 2024

sourcery-ai bot Mar 4, 2024

sdobbelaere commented Mar 4, 2024

fabien-michel commented Mar 4, 2024 •

edited

Loading

bellini666 left a comment

bellini666 commented Mar 4, 2024

fabien-michel commented Mar 5, 2024 •

edited

Loading

sdobbelaere commented Mar 5, 2024 •

edited

Loading

bellini666 commented Mar 5, 2024

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

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

Conversation

fabien-michel commented Mar 4, 2024 • edited by bellini666 Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Mar 4, 2024

Choose a reason for hiding this comment

sourcery-ai bot Mar 4, 2024

Choose a reason for hiding this comment

sourcery-ai bot Mar 4, 2024

Choose a reason for hiding this comment

sourcery-ai bot Mar 4, 2024

Choose a reason for hiding this comment

sourcery-ai bot Mar 4, 2024

Choose a reason for hiding this comment

sdobbelaere commented Mar 4, 2024

fabien-michel commented Mar 4, 2024 • edited Loading

bellini666 left a comment

Choose a reason for hiding this comment

bellini666 commented Mar 4, 2024

fabien-michel commented Mar 5, 2024 • edited Loading

sdobbelaere commented Mar 5, 2024 • edited Loading

bellini666 commented Mar 5, 2024

fabien-michel commented Mar 4, 2024 •

edited by bellini666

Loading

fabien-michel commented Mar 4, 2024 •

edited

Loading

fabien-michel commented Mar 5, 2024 •

edited

Loading

sdobbelaere commented Mar 5, 2024 •

edited

Loading