Skip to content

fix: remove if-wrappers from error flags#369

Merged
toddbaert merged 1 commit intomainfrom
fix/remove-if-wrappers-operator-errors
Apr 20, 2026
Merged

fix: remove if-wrappers from error flags#369
toddbaert merged 1 commit intomainfrom
fix/remove-if-wrappers-operator-errors

Conversation

@toddbaert
Copy link
Copy Markdown
Member

The if wrappers were added in #356 (my bad), but json-logic's if coerces its condition to a boolean; null and false are both falsy, so the if always selects the "false" variant for both cases. This makes it impossible to verify that operators return null (not false) on error, which is the entire point of the @operator-errors scenarios.

By using the operator directly as the top-level targeting expression, a null return propagates to the evaluator, which falls back to defaultVariant with reason DEFAULT; a false return would resolve to the "false" variant with reason TARGETING_MATCH. The distinction is preserved.


I tested this locally and with this and the expected fixes, I was able to get the test suite fully updated in in-process mode! No more inconsistencies!

@toddbaert toddbaert requested a review from a team as a code owner April 20, 2026 20:32
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert force-pushed the fix/remove-if-wrappers-operator-errors branch from 4ddeaeb to 52752bf Compare April 20, 2026 20:34
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the flag configuration files by simplifying the targeting logic, specifically removing redundant 'if' wrappers and directly defining conditions like 'sem_ver', 'fractional', and string operators. It also updates the variants for fractional flags to better match their bucket configurations. I have no feedback to provide.

@toddbaert toddbaert merged commit e40714e into main Apr 20, 2026
3 checks passed
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.

2 participants