-
Notifications
You must be signed in to change notification settings - Fork 179
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
Detect wrong-typed constraints #1246
Detect wrong-typed constraints #1246
Conversation
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.
Very nice, @thomaslevesque. I did make a couple of minor comments. If you agree and implement the suggested changes, feel free to squash/amend as you go.
@@ -20,4 +22,9 @@ internal interface IArgumentConstraint | |||
/// <returns>True if the argument is valid.</returns> | |||
bool IsValid(object argument); | |||
} | |||
|
|||
internal interface ITypedArgumentConstraint : IArgumentConstraint |
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.
Should be in a separate file, no?
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.
Sure
"Then the call configuration throws a FakeConfigurationException" | ||
.x(() => exception.Should().BeAnExceptionOfType<FakeConfigurationException>()); | ||
|
||
"And the message should indicate the actual and expected type" |
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.
nit: previous steps used the imperative mood, not the subjunctive (fun fact: I would probably not even know these moods exist were it not for learning about "le subjontif" in school).
Consider rewording. Maybe something like "And the message indicates the actual and expected types"
(oh, I changed "type" to "types" there as well)
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
} | ||
|
||
[Scenario] | ||
public static void PassingHiddenConstraintWithWrongTypeToAMethod( |
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 like this test.
d472eea
to
be30855
Compare
be30855
to
bf8d8c2
Compare
The rebase was a bit messy, hopefully I didn't break anything... |
Nice, @thomaslevesque! Thanks. |
Thanks for the merge! |
Fixes #1237
The 3rd commit is a simplification. I realized that only
MatchesConstraint
could be created by the argument constraint trap, so we don't need to introduce aType
property for other types of constraint.