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(python): Options that are None are also None in python #212

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bennettandrews
Copy link

Description

Options that are None in rust are also None in python instead of empty tuples.

Currently when it is converted back to python it ends up as tuple () which will create an empty tuple object instead.

Testing

Updated test to include checking conversion.

References

fixes: #211

Copy link

changeset-bot bot commented Feb 21, 2025

🦋 Changeset detected

Latest commit: 204ac7c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
eppo_core Patch
python-sdk Patch
ruby-sdk Patch
rust-sdk Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rasendubi
Copy link
Collaborator

Thanks for debugging and fixing this! Extra kudos for adding tests ❤️

@rasendubi
Copy link
Collaborator

Hm... it looks like the test is failing:

>       assert event["subjectAttributes"]["empty"] is None
E       assert () is None

Did you get it passing locally?

This should fix `null` attributes serializing to unit instead of none.
@rasendubi
Copy link
Collaborator

Ok, I figured it out. The bug is coming from AttributeValue having bad implementation for serde::Serialize. I've fixed it and added a commit to your branch

@bennettandrews
Copy link
Author

@rasendubi Thanks. I was still working on figuring out some pieces of getting local builds to work so I wasn't able to test this yet.

Copy link
Member

@leoromanovsky leoromanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find thank you! The other CI check is failing and will be fixed in the MSRV pr here (#213).

We'll get this packaged up and shipped for you early next week - cheers!

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.

Empty subject attributes in python are empty tuples instead of None
3 participants