-
-
Notifications
You must be signed in to change notification settings - Fork 316
Allow a custom equals parameter for observable collections #970
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
base: main
Are you sure you want to change the base?
Conversation
👷 Deploy request for mobx pending review.Visit the deploys page to approve it
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #970 +/- ##
==========================================
- Coverage 99.00% 98.38% -0.62%
==========================================
Files 57 57
Lines 2005 2043 +38
==========================================
+ Hits 1985 2010 +25
- Misses 20 33 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
076d509
to
851e078
Compare
851e078
to
d13d88d
Compare
b1f29d8
to
fda5217
Compare
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.
Pull Request Overview
This PR allows custom equality functions to be specified for MobX observable collections (ObservableList, ObservableMap, and ObservableSet). This enables developers to define custom comparison logic for determining when values are considered equal, which can optimize change detection and support custom equality semantics.
- Adds
EqualityComparer<T>? equals
parameter to all observable collection constructors - Implements custom equality logic in value comparison operations for all three collection types
- Updates
cast
methods to preserve custom equality functions with optional overrides
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
mobx/test/observable_collections_equals_test.dart | Comprehensive test suite covering custom equals functionality across all collection types |
mobx/pubspec.yaml | Version bump from 2.5.0 to 2.6.0 for minor feature addition |
mobx/lib/version.dart | Updates version constant to match pubspec |
mobx/lib/src/api/observable_collections/observable_set.dart | Implements custom equals support for ObservableSet with updated constructors and equality logic |
mobx/lib/src/api/observable_collections/observable_map.dart | Adds custom equals parameter for value comparisons in ObservableMap |
mobx/lib/src/api/observable_collections/observable_list.dart | Integrates custom equals functionality into ObservableList operations |
mobx/lib/src/api/observable_collections.dart | Adds required imports for collection package and utils |
mobx/CHANGELOG.md | Documents the new custom equals feature in version 2.6.0 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
} | ||
|
||
T? _findValue(Object? value) { | ||
if (value is T?) { |
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.
The type check value is T?
is incorrect for non-nullable types T. When T is non-nullable (e.g., String
), T?
becomes String?
, but value
with type Object?
cannot be checked against T?
directly. This should use value is T
to check if value can be cast to the non-nullable type T.
if (value is T?) { | |
if (value is T) { |
Copilot uses AI. Check for mistakes.
_list.cast<R>(), | ||
equals ?? | ||
(_equals != null | ||
? (R? a, R? b) => _equals!(a as T?, b as T?) |
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.
Unsafe casting from R? to T? without type checking. If R and T are incompatible types, this will cause a runtime exception. Should add type compatibility checks or use a safer casting approach.
? (R? a, R? b) => _equals!(a as T?, b as T?) | |
? (R? a, R? b) { | |
if (a is T? && b is T?) { | |
return _equals!(a, b); | |
} | |
return false; | |
} |
Copilot uses AI. Check for mistakes.
equals ?? | ||
(_equals == null | ||
? null | ||
: (RV? a, RV? b) => _equals!(a as V?, b as V?))); |
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.
Unsafe casting from RV? to V? without type checking. If RV and V are incompatible types, this will cause a runtime exception. Should add type compatibility checks or use a safer casting approach.
: (RV? a, RV? b) => _equals!(a as V?, b as V?))); | |
: (RV? a, RV? b) { | |
if (a is V? && b is V?) { | |
return _equals!(a, b); | |
} | |
// If types are incompatible, consider them not equal | |
return false; | |
})); |
Copilot uses AI. Check for mistakes.
equals ?? | ||
(_equals == null | ||
? null | ||
: (R? a, R? b) => _equals!(a as T?, b as T?))); |
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.
Unsafe casting from R? to T? without type checking. If R and T are incompatible types, this will cause a runtime exception. Should add type compatibility checks or use a safer casting approach.
: (R? a, R? b) => _equals!(a as T?, b as T?))); | |
: (R? a, R? b) { | |
if (a is! T || b is! T) return false; | |
return _equals!(a as T?, b as T?); | |
})); |
Copilot uses AI. Check for mistakes.
Describe the changes proposed in this Pull Request.
Allow a custom equals parameter for observable collections, like observable stream, observable and computed .
#771
#482
If the PR fixes a specific issue, reference the issue with
Fixes #
.Pull Request Checklist
pubspec.yaml
is updated.major
/minor
/patch
/patch-count
, depending on the complexity of changemelos run set_version
command from the root directory