Skip to content
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.

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

Fix compilation of fields with name identical to their type #294

Merged
merged 3 commits into from
Dec 1, 2021
Merged

Conversation

guysz
Copy link
Contributor

@guysz guysz commented Nov 30, 2021

Defining a dataclass with a field that its name is identical to its type name, and is initialized as a field produces RecursionError:

from dataclasses import dataclass, field
@dataclass
class Foo:
    bool: bool = field()

This PR suggests a solution to the problem by using built-in types from the builtins package, so the above code will be changed to:

from dataclasses import dataclass, field
import builtins
@dataclass
class Foo:
   bool: builtints.bool = field()

This PR also reverts #226 because it is solving a more general problem.

@Gobot1234
Copy link
Collaborator

I'm not a huge fan of this for two reasons:

  1. This makes the code less pythonic by default.
  2. This is not an issue that the library by default should have to handle.

Fixing the first one is relatively easy, only add builtins if a name in builtins is a field name, I'll give the second a go so this can also be version conditional in the future.

@Gobot1234
Copy link
Collaborator

I've opened an issue on the bpo https://bugs.python.org/issue45946 about this.

@guysz
Copy link
Contributor Author

guysz commented Dec 1, 2021

  1. Fixed - add builtins. prefix only when needed.
  2. Why do you think the library should not handle such a case? In practice, the tool generates a broken code from a valid proto file. Specifically, in my use-case, I use a third-party proto definition that falls into this issue which means I can't use this tool without the fix (and I really like to use it :))

@Gobot1234
Copy link
Collaborator

I'm saying in an ideal world this shouldn't be fixed by us but by cpython.

@Gobot1234
Copy link
Collaborator

Gobot1234 commented Dec 1, 2021

Can you also add a test for this?

@Gobot1234
Copy link
Collaborator

Assuming tests pass this looks much better than the original solution to this. Thank you!

@Gobot1234 Gobot1234 merged commit b0a36d1 into danielgtaylor:master Dec 1, 2021
Gobot1234 added a commit to Gobot1234/python-betterproto that referenced this pull request Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants