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 to_(py)?dict bool fields #494

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alirezasafi
Copy link

@alirezasafi alirezasafi commented May 27, 2023

Fixes #490

@Gobot1234 Gobot1234 changed the title Fix to-pydict function. (Issue #490) Fix to_pydict with optional fields May 27, 2023
@Gobot1234
Copy link
Collaborator

Hi, thank you for the PR, would you be able to run poe fmt and add a regression test for this code change?

@Gobot1234 Gobot1234 changed the title Fix to_pydict with optional fields Fix to_(py)?dict bool fields May 27, 2023
@Gobot1234
Copy link
Collaborator

I'm very hesitant to make a change of behaviour like this just for bool fields. Does the proposed change apply to all fields which are set and include them even if bool(value) returns False?

@alirezasafi
Copy link
Author

alirezasafi commented May 27, 2023

yes its applied to all of them and include bool field even if the value is False.
here is the sample code:

from dataclasses import dataclass

import betterproto


@dataclass
class Greeting(betterproto.Message):
    """Greeting represents a message you can tell a user."""

    int_field: int = betterproto.int32_field(1)
    bool_field: bool = betterproto.bool_field(2)


if __name__ == "__main__":
    greeting = Greeting(
        bool_field=False,
    )
    print("default value is disabled (bool_field=False): ", greeting.to_pydict(include_default_values=False))
    print("default value is enabled (bool_field=False): ", greeting.to_pydict(include_default_values=True))
    greeting = Greeting()
    print("default value is disabled (unset bool_field): ", greeting.to_pydict(include_default_values=False))
    print("default value is enabled (unset bool_field): ", greeting.to_pydict(include_default_values=True))

output:

default value is disabled (bool_field=False):  {'boolField': False}
default value is enabled (bool_field=False):  {'intField': 0, 'boolField': False}
default value is disabled (unset bool_field):  {}
default value is enabled (unset bool_field):  {'intField': 0, 'boolField': False}

@Gobot1234
Copy link
Collaborator

Sorry if I wasn't clear, I meant what's the output for things that implement bool that aren't int subclasses. ie what does

from dataclasses import dataclass

import betterproto

@dataclass
class Msg(bettterproto.Message):
    bar: bool = betterproto.bool_field(1)

@dataclass
class Greeting(betterproto.Message):
    """Greeting represents a message you can tell a user."""

    msg_field: Msg = betterproto.message_field(1)
    str_field: str = betterproto.string_field(2)


if __name__ == "__main__":
    greeting = Greeting(
        Msg(), ""
    )
    print(greeting.to_pydict())

print?

@alirezasafi
Copy link
Author

from dataclasses import dataclass

import betterproto

@dataclass
class Msg(bettterproto.Message):
    bar: bool = betterproto.bool_field(1)

@dataclass
class Greeting(betterproto.Message):
    """Greeting represents a message you can tell a user."""

    msg_field: Msg = betterproto.message_field(1)
    str_field: str = betterproto.string_field(2)


if __name__ == "__main__":
    greeting = Greeting(
        Msg(), ""
    )
    print(greeting.to_pydict())

only strField is in the output:

{'strField': ''}

@Gobot1234
Copy link
Collaborator

Interesting, I would expect an empty dict for the msg field cause it's set

@alirezasafi
Copy link
Author

alirezasafi commented May 27, 2023

bool_field

why?
i think the output is correct. because the bool field bar didn't set and default value disabled.

@Gobot1234
Copy link
Collaborator

I'd expect the output to be the output you've shown if the Greeting(str_field="") but according to proto spec setting the value of msg_field still means it should show up in the wire format so I think the output should be {'msgField': Msg(), 'strField': ''} for the first input I gave in #494 (comment)

@Gobot1234
Copy link
Collaborator

Please could you add some unit tests for this feature please

@cetanu
Copy link
Collaborator

cetanu commented Oct 16, 2023

I'd expect the output to be the output you've shown if the Greeting(str_field="") but according to proto spec setting the value of msg_field still means it should show up in the wire format so I think the output should be {'msgField': Msg(), 'strField': ''} for the first input I gave in #494 (comment)

I agree, if a message is placed into a field, I would consider it set and expect there to be an empty dict there, if the message itself has no fields set.

@Gobot1234 Gobot1234 self-assigned this Oct 30, 2023
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.

to_dict or to_pydict miss integer and boolean fields
3 participants