Skip to content
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

Adapt CtorParameterAndPropertySetterExists test for v2.3 behavior #1156

Merged
merged 3 commits into from Jan 2, 2021

Conversation

AArnott
Copy link
Collaborator

@AArnott AArnott commented Dec 21, 2020

See also #1155

CC: @pCYSl5EDgo who introduced this breaking change. Was the breaking change necessary for what you were delivering (#1095)?

@AArnott AArnott added this to the v2.3 milestone Dec 21, 2020
@AArnott AArnott requested a review from neuecc December 21, 2020 19:49
@AArnott AArnott self-assigned this Dec 21, 2020
@pCYSl5EDgo
Copy link
Contributor

Oh, I was not aware of that this breaking change caused this situation!
Thank you for pointing out!

I introduced this breaking change in order not to set parameters twice(constructor and setter).

@AArnott
Copy link
Collaborator Author

AArnott commented Dec 23, 2020

IMO setting them twice is wasteful and the new behavior is better. But I am less sure it is worth the behavioral change. Perhaps we can keep it for now and revert this part of your change if folks complain.

@pCYSl5EDgo
Copy link
Contributor

I agree with the reverting of this behaviour when folks complain it.

@AArnott AArnott merged commit 4ca7e42 into MessagePack-CSharp:develop Jan 2, 2021
@AArnott AArnott deleted the docCtorPlusSetter_v2.3 branch January 2, 2021 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants