-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Implement IDisposable for ConvertToJsonCommand
#15787
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
daxian-dbw
left a comment
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. I made a minor update to reduce the nesting.
|
@daxian-dbw I looked at the implementation again, and removed the unnecessary
Feel free to revert to reset to 29cdfb4 if you disagree. |
|
@xtqqczze In that case, you don't really need to follow the exact |
@daxian-dbw I already considered those changes, but they are only possible if the class is sealed. Since |
|
The sealed class issue reminds me of comments in #11820 (review). |
|
🎉 Handy links: |
Fix #15740.
cc: @iSazonov