Moving from raw instance check to name checks in findBreakingArgChanges
#800
Conversation
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
|
Thanks for the quick fix! You're right that a unit test would be very useful here. |
|
I'll stew over that and fix the flow issues as well... |
Awesome, thanks! |
joelgriffith
commented
Apr 12, 2017
| } | ||
| ]); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test asserts were not using instance equality to validate that an argument has changed in a breaking way (this test will fail if so)
|
I've verified that this works in another project as expected. |
joelgriffith
changed the title
findBreakingArgChanges
|
Looks good. Thanks for this. Will cut a release shortly to get this fix out. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Addresses #783. When I initially wrote the
findBreakingArgChanges, doing comparison checks on the arg instances themselves was the state-of-the-art. It looks like thefindFieldsThatChangedTypenow references thenameproperty of the types to check if changes happened in a breaking way, which is what I've done in this PR.Not sure how I can assert this correction in unit-tests... would be happy for feedback