Skip to content

feat: Add In operator edge cases - #38

Open
khvn26 with Copilot wants to merge 4 commits into
mainfrom
copilot/add-in-operator-edge-cases
Open

feat: Add In operator edge cases#38
khvn26 with Copilot wants to merge 4 commits into
mainfrom
copilot/add-in-operator-edge-cases

Conversation

CopilotAI commented Oct 27, 2025

Copy link
Copy Markdown
Contributor

Added 12 new test cases for IN operator edge cases:

Native list condition properties:

  • float context value vs float[] condition property — should match
  • int context value vs int[] condition property — should match
  • float context value vs int[] condition property — should not match
  • int context value vs float[] condition property — should not match

Comma-delimited string condition properties:

  • float context value vs string comma-delimited floats condition property — should match
  • int context value vs string comma-delimited ints condition property — should match
  • float context value vs string comma-delimited ints condition property — should not match
  • int context value vs string comma-delimited floats condition property — should not match

JSON-encoded list condition properties:

  • float context value vs string JSON-encoded floats condition property — should match
  • int context value vs string JSON-encoded ints condition property — should match
  • float context value vs string JSON-encoded ints condition property — should not match
  • int context value vs string JSON-encoded floats condition property — should not match

All test cases follow the existing naming convention and structure, with descriptive comments explaining each scenario. Float values use 2.0 (vs int 2) to test ambiguous cases that are equal in loose comparison but distinct in type-strict comparison. Native array values are now strings to comply with the JSON schema.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: khvn26 <979078+khvn26@users.noreply.github.com>
CopilotAI changed the title [WIP] Add edge cases for In operator handlingAdd IN operator edge cases for int/float type matchingOct 27, 2025
CopilotAI requested a review from khvn26October 27, 2025 17:18
@khvn26

Copy link
Copy Markdown
Member

@copilot for test cases involving floats, we should use values that add ambiguity when loosely compared — i.e. 2 vs 2.0, not 2.5.

… vs 2)
Co-authored-by: khvn26 <979078+khvn26@users.noreply.github.com>

CopilotAI commented Oct 27, 2025

Copy link
Copy Markdown
ContributorAuthor

@copilot for test cases involving floats, we should use values that add ambiguity when loosely compared — i.e. 2 vs 2.0, not 2.5.

Updated all float test cases to use 2.0 instead of 2.5 for better ambiguity testing. Commit: 2abf80b

@khvn26khvn26 changed the title Add IN operator edge cases for int/float type matchingfeat: Add In operator edge casesOct 27, 2025
@khvn26
khvn26 marked this pull request as ready for review October 27, 2025 17:30
@khvn26
khvn26 requested a review from a team as a code ownerOctober 27, 2025 17:30
khvn26

This comment was marked as outdated.

@khvn26
khvn26 requested review from a team and Zaimwa9 and removed request for a teamOctober 27, 2025 17:30
gagantrivedi
gagantrivedi previously approved these changes Oct 29, 2025
@khvn26
khvn26 self-requested a review October 29, 2025 09:00
@khvn26

Copy link
Copy Markdown
Member

@copilot make sure the new test cases are compliant with their JSON schema — "value" array values should be arrays of (numeric) strings.

Co-authored-by: khvn26 <979078+khvn26@users.noreply.github.com>

CopilotAI commented Oct 29, 2025

Copy link
Copy Markdown
ContributorAuthor

@copilot make sure the new test cases are compliant with their JSON schema — "value" array values should be arrays of (numeric) strings.

Updated native array values to use strings (e.g., ["1.0", "2.0", "3.0", "4.0"] instead of [1.0, 2.0, 3.0, 4.0]) to comply with the JSON schema. Commit: c8ed747

{
"operator": "IN",
"property": "level",
"value": "[1,2,3,4]"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is still a list of integer?

@khvn26khvn26Oct 31, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Currently, we expect the engine to coerce the unmarshalled json-encoded values to an array of strings — see https://github.com/Flagsmith/flagsmith-engine/blob/3c28ed5a578c5782c3f289c51cab97cb0f4d6c09/flag_engine/segments/evaluator.py#L277-L287.

Is this a concern for Go and Rust?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a concern for Rust. When converting floats to strings, Rust removes trailing zeros—for example, 2.0.to_string() returns "2" instead of "2.0". This causes the IN operator tests to fail because the string representation of the float trait value ("2")
doesn't match the comma-delimited float values in the condition ("1.0,2.0,3.0,4.0").

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We've taken the discussion offline and couldn't come to a good conclusion yet — created #44 to address this.

Sign up for freeto 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

@khvn26@gagantrivedi