Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

FF-3106 fix: ContextAttributes.from_dict() should not treat bool as numeric #67

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

rasendubi
Copy link
Collaborator


labels: mergeable

Fixes: https://linear.app/eppo/issue/FF-3106/%5Bpython%5D-contextattributesfrom-dict-adds-bool-attributes-to-both

Motivation and Context

The added test case was failing because bool was added to both categorical (as "true") and numeric (as 1.0) attributes. This happened because bool is a subtype of int in Python, so isinstance(True, int) == True.

Description

And an explicit check that attribute is not bool in categorical attributes filter.

How has this been tested?

Added a test case.

@schmit
Copy link
Contributor

schmit commented Aug 28, 2024

This happened because bool is a subtype of int in Python

🤯 I thought this kind of stuff only happens in JS 🤣

Copy link
Contributor

@schmit schmit left a comment

Choose a reason for hiding this comment

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

Fantastic catch

@rasendubi rasendubi force-pushed the ff-3106-contextattributes_fromdict_bool_as_numeric branch from 25e5f8d to 591e1cf Compare August 29, 2024 17:01
@@ -1,4 +1,4 @@
# Note to developers: When ready to bump to 4.0, please change
# the `POLL_INTERVAL_SECONDS` constant in `eppo_client/constants.py`
# to 30 seconds to match the behavior of the other server SDKs.
__version__ = "3.5.3"
__version__ = "3.5.4"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bumped the version as 3.5.3 was just released

@rasendubi rasendubi merged commit b4340bb into main Aug 29, 2024
4 checks passed
@rasendubi rasendubi deleted the ff-3106-contextattributes_fromdict_bool_as_numeric branch August 29, 2024 17:01
Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

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

Nice catch! Ideally we promote your test case to the shared test cases repository

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants