Add missing equality checks to MethodSignatureComparer.EqualSignatureTypes#310
Merged
jonorossi merged 4 commits intocastleproject:masterfrom Oct 9, 2017
Merged
Conversation
stakx
force-pushed
the
methodsignaturecomparer-bug
branch
from
9b4f814 to
a2efa6b
Compare
Given two generic types, `EqualSignatureTypes` only checks whether their generic type arguments match, but it doesn't check whether the actual type matches. This commits adds those missing checks.
stakx
force-pushed
the
methodsignaturecomparer-bug
branch
from
a2efa6b to
33ae41a
Compare
stakx
commented
Oct 4, 2017
|
|
||
| private class GenericClass1<T> { } | ||
|
|
||
| private class GenericClass2<T> { } |
Member
Author
There was a problem hiding this comment.
I felt it was important to keep the two tests separate:
- The unit test here deals with some internal cogwheel;
- The regression unit test acts at the level of the public API.
Yet, declaring the same set of test types (the interface, plus GenericClass1<T> and GenericClass2<T>) twice seems somewhat repetitive. Do you mind? (If so, where would be the best place to put them?)
Member
There was a problem hiding this comment.
Yet, declaring the same set of test types (the interface, plus GenericClass1 and GenericClass2) twice seems somewhat repetitive. Do you mind? (If so, where would be the best place to put them?)
Two tests sounds fine to me, means if the internal implementation changes we'll at least still have the regression one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Given two generic types,
EqualSignatureTypesonly checks whether their generic type arguments match, but it doesn't check whether the actual generic type definition (i.e. the "open" generic type) matches. This PR adds those missing checks.This should fix #309.
Please beware: I was a bit pressed for time when I put this PR together, so my understanding of
MethodSignatureComparer's purpose and exact uses is incomplete. I've relied mostly on the unit test suite to tell me whether I'm introducing any breaking change or not.