-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ParameterState: Add optional IEqualityComparer #8416
Conversation
@henon missing unit + bunit tests, which i will add later, but the feature itself is ready to be reviewed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #8416 +/- ##
==========================================
- Coverage 88.88% 88.75% -0.14%
==========================================
Files 414 416 +2
Lines 12294 12372 +78
Branches 2455 2459 +4
==========================================
+ Hits 10928 10981 +53
- Misses 836 860 +24
- Partials 530 531 +1 ☔ View full report in Codecov by Sentry. |
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 builder makes a lot of sense. I didn't know it at the time but the gut decision for the RegisterParameter
method in the component constructor was the right one, seeing that it is now able to abstract a lot of complexity away from the framework user.
After reading our CONTRIBUTING.md, people still seem to be managing to shoot themselves in the foot with the framework. I will think a bit on how to improve the docs without making them overly long and exhausting to read.
In this PR I also moved the
I also added tests for |
Added bUnit test. Merging as the feature is complete and fully tested. |
@henon now when I look at some components I think I made a mistake and the [Parameter]
[Category(CategoryTypes.TreeView.Selecting)]
public IEqualityComparer<T?> Comparer { get; set; } = EqualityComparer<T?>.Default; And it's used for the |
Agreed |
Description
Fixes when custom comparison is required.
Replaced non generic
ParameterState
with a builder.Builder pattern is really good when overloads grow exponentially, so I had to drop the non generic
ParameterState
helper for the builder pattern. While it's still manageable forRegisterParameters
it wasn't the case for theParameterState
helper.It's much easier to use now
ParameterAttachBuilder
, it has own drawbacks like you need to set theT
explicitly, it can't get it from the context, BUTParameterAttachBuilder
should not be used by anyone, not by users and not by contributors, it's exclusively for the tests and theRegisterParameters
to build complexParameterState<T>
which has so much optional arguments and in future it may only grow.How Has This Been Tested?
None as for now, unit tests are work in progress
Types of changes
Checklist:
dev
).