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

convert NamedTuples to @dataclass #38

Open
jonabox opened this issue Sep 12, 2021 · 4 comments
Open

convert NamedTuples to @dataclass #38

jonabox opened this issue Sep 12, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@jonabox
Copy link

jonabox commented Sep 12, 2021

Hi, I'm creating a frontend wrapper for this project and noticed that NamedTuple and @dataclasses are used almost interchangeably in model.py. I was wondering why that's the case and if it would be possible to just change all NamedTuples to @dataclasses. This would make the model more consistent. I tried changing it for a couple classes and didn't notice anything breaking.

@blumu
Copy link
Contributor

blumu commented Sep 14, 2021

If I recall correctly, we favor NamedTuple whenever possible so that the underlying data structure is guaranteed to be immutable by default. We use @dataclasses when mutability is required.
Replacing NamedTuple by @dataclasses may not alter the program behavior though it may prevent the type-checker from detecting potential bugs caused by mutations.

That said if that causes problems for the wrapper you are writing, we could change this convention. I see that @dataclasses were introduced in python 3.7 and that they also support immutability via the frozen flag, so I wonder if it was meant to fully replace NamedTuple though the doc does not clarify: https://docs.python.org/3/library/dataclasses.html

@jonabox
Copy link
Author

jonabox commented Sep 14, 2021

Ahh, that's great reasoning. A big part of the project I'm working on is letting users modify generated models such as adding nodes, updating vulnerability descriptions, etc. This would be easier (albeit riskier) to do with mutable @dataclasses but perhaps frozen @dataclasses would strike a good balance.

Also, @dataclasses have been a lot easier for me to serialize into JSON as opposed to NamedTuples, which did not yield me good results. It might be that they were not designed with that functionality in mind. On my end, I had to change a couple NamedTuples into @dataclasses to be able to send data over to the frontend. (I tried using the YAML serializer but that brought me a few problems, unfortunately.)

@blumu
Copy link
Contributor

blumu commented Dec 17, 2021

@jonabox Have you tried the orjson serialization package (https://github.com/ijl/orjson/) mentioned in one of the answers to the stackoverflow article you cited? According to the doc it handles NameTuples correctly.

@blumu blumu added the enhancement New feature or request label Dec 17, 2021
@jonabox
Copy link
Author

jonabox commented Dec 17, 2021

Not quite, I ended up changing NamedTuples into @dataclasses(), some of them frozen. I then added an encode method to these classes to fine-control the json.dumps() output. For example:

@dataclass(frozen=True)
class CachedCredential():
    """Encodes a machine-port-credential triplet"""
    node: NodeID
    port: PortName
    credential: CredentialID

    def encode(self):
        return dataclasses.asdict(self)

cachedCredential = CachedCredential(node="client", port="TCP",  credential="TCP-credential" )
json.dumps(cachedCredential, default=lambda x: x.encode())
# returns {"node": "client", "port": "TCP",  "credential": "TCP-credential"}

I might still try orjson, and get back with any findings, but those slight modifications have worked really well for me so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants