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

Merge some metadata classes #753

Merged
merged 1 commit into from
Oct 19, 2020
Merged

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Oct 8, 2020

Another small set of merges. No noteworthy changes.

Copy link
Member

@cheenamalhotra cheenamalhotra left a 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 class SmiRecordBuffer, implemented in TdsRecordSetterBuffer.cs but is unused, seems to be safe for deletion, as netcore version of files are chosen.

@cheenamalhotra
Copy link
Member

Just FYI..
We're looking into pipeline trigger issues recently happening, will let you know when we'd need a push on this branch to trigger CI builds again.

@cheenamalhotra
Copy link
Member

The pipelines are back, could you try pushing an update to your PR?
e.g. "merge" from "dotnet/master"?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 9, 2020

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...

@cheenamalhotra
Copy link
Member

Yep I got that, just wanted to call out changes in case that raises questions.
FYI, the pipelines were not triggered, you'll have to check-in something for checks to run.

@Wraith2 Wraith2 closed this Oct 9, 2020
@Wraith2 Wraith2 reopened this Oct 9, 2020
@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 9, 2020

turned it off and then turned it on again, that triggered it 😁

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 12, 2020

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.

@cheenamalhotra cheenamalhotra added this to the 2.1.0-preview2 milestone Oct 16, 2020
@JRahnama
Copy link
Member

LGTM.

@cheenamalhotra cheenamalhotra merged commit b9a96d8 into dotnet:master Oct 19, 2020
@Wraith2 Wraith2 deleted the combine7 branch October 22, 2020 23:14
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.

4 participants