-
Notifications
You must be signed in to change notification settings - Fork 293
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
Merge some metadata classes #753
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.
LGTM!
Small note for reviewing team:
- There's a virtual
Close
method in netfx classSmiRecordBuffer
, implemented inTdsRecordSetterBuffer.cs
but is unused, seems to be safe for deletion, as netcore version of files are chosen.
Just FYI.. |
The pipelines are back, could you try pushing an update to your PR? |
I looked at the Close implementation and originally pulled it into the netcore version but later tracing through what it did I realised it was totally dead code and re-removed it. So I did the due diligence even though it looks like I just took netcore. Pipelines seem to be passed now... |
Yep I got that, just wanted to call out changes in case that raises questions. |
turned it off and then turned it on again, that triggered it 😁 |
The test failures are from a changed localized message that's being tested for directly. That seems a bit fragile and it isn't related to the changes that I've made in this PR. |
LGTM. |
Another small set of merges. No noteworthy changes.