-
Notifications
You must be signed in to change notification settings - Fork 504
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 PartialEq for Value in opentelemetry-stdout #1955
Conversation
PartialEq's previous implementation called back to itself with the same arguments. Here we extract the underlying value and compare them. If the enum types are different, it returns false.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1955 +/- ##
=======================================
- Coverage 74.9% 74.9% -0.1%
=======================================
Files 122 122
Lines 20375 20387 +12
=======================================
Hits 15276 15276
- Misses 5099 5111 +12 ☔ View full report in Codecov by Sentry. |
(Value::Array(a), Value::Array(oa)) => a.eq(oa), | ||
(Value::KeyValues(kv), Value::KeyValues(okv)) => kv.eq(okv), | ||
(Value::BytesValue(b), Value::BytesValue(ob)) => b.eq(ob), | ||
(Value::Bool(_), _) => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a simpler wild card pattern instead?
_ => false,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern was that if a new variant were added to Value
, PartialEq
would compile but have an incorrect result. With this version, there would be a compile error. But if you favor conciseness, I can change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
Fixes #1952
Changes
PartialEq's previous implementation could call itself again with the same arguments. Here the underlying values are extracted and compared. If the enum types are different, false is returned.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes