-
Notifications
You must be signed in to change notification settings - Fork 747
Allows Is.AnyOf to be called with arrays or other collections #4356
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
Conversation
That's....better, yeah. 😆 |
/// </summary> | ||
protected virtual string GetStringRepresentation() | ||
protected string GetStringRepresentation(IEnumerable arguments) |
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.
question: Since Constraint
is public, this overload technically becomes a new "public" API as a custom derived Constraint may try to call this. I usually default to protected
in this case out of habit, but figured I'd ask: would private protected
be wanted here instead?
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.
I had to look up private protected
because it logically didn't make sense. Since when does private
mean internal
?
But that is because protected internal
means either derived (anywhere)
or direct calls from the assembly
instead of only derived classes inside the assembly
I think this can be protected
in case an external Constraint
has a single IEnumerable
argument which by default gets formatted as just the type name.
There are at least two existing constraints: CollectionSubsetConstraint
and CollectionSupersetConstraint
which could benefit from this call as their current (tested against) string representation is: <s...setof System.Int32[]>
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.
Nicely done, this looks good to me. Thanks @manfred-brands
I'd like to leave it open for another team member to take a look too, just in case I'm missing anything. I left a question about member visibility
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.
Also looks good to me, and @manfred-brands reasoning in the PR and the issue seems valid (at least he has given it more thought/examination than I have).
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.
LGTM too.
Hey how about
I want Has.All.AnyOf() :) |
Fixes #4355