Skip to content

Merge some metadata classes - #753

Merged
cheenamalhotra merged 1 commit into
dotnet:masterfrom
Wraith2:combine7
Oct 19, 2020
Merged

Merge some metadata classes#753
cheenamalhotra merged 1 commit into
dotnet:masterfrom
Wraith2:combine7

Conversation

@Wraith2

Copy link
Copy Markdown
Contributor

Another small set of merges. No noteworthy changes.

@cheenamalhotracheenamalhotra left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

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

@Wraith2

Copy link
Copy Markdown
ContributorAuthor

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

@Wraith2Wraith2 closed this Oct 9, 2020
@Wraith2Wraith2 reopened this Oct 9, 2020
@Wraith2

Copy link
Copy Markdown
ContributorAuthor

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

@Wraith2

Copy link
Copy Markdown
ContributorAuthor

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.

@cheenamalhotracheenamalhotra added this to the 2.1.0-preview2 milestone Oct 16, 2020
@JRahnama

Copy link
Copy Markdown
Contributor

LGTM.

Sign up for freeto 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

@Wraith2@cheenamalhotra@JRahnama@DavoudEshtehari