Skip to content

Conversation

@WilliamAntonRohm
Copy link
Contributor

@WilliamAntonRohm WilliamAntonRohm commented Jul 26, 2019

per work item https://dev.azure.com/mseng/TechnicalContent/_workitems/edit/1558837/

related samples PR is dotnet/samples#1115

applies to:
.ctor
Add
Capacity
Clear
Count
Insert
Remove
TrimExcess

@WilliamAntonRohm WilliamAntonRohm changed the title Try.NET update for system.collections.generic.list-1.add Try. NET update for system.collections.generic.list-1.add Jul 26, 2019
@WilliamAntonRohm
Copy link
Contributor Author

@BillWagner -- Bill, please review this article update alongside sample PR 1115

@mairaw -- fyi

@mairaw
Copy link
Contributor

mairaw commented Jul 26, 2019

We'll start the review soon. You can ping me now since I'm back from vacation already.

@mairaw mairaw closed this Jul 29, 2019
@mairaw mairaw reopened this Jul 29, 2019
@mairaw
Copy link
Contributor

mairaw commented Jul 29, 2019

Reopening to see if the new sample shows up.

Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @WilliamAntonRohm. You called this PR for list-1.add but you've actually done it for list-1.capacity, which is fine but not sure if this the one you wanted to test. Also, on the spreadsheet several APIs might fall on the same xml file, so you might want to combine them in one PR.
I also left a comment on where the note should be placed.

@mairaw mairaw added this to the August 2019 milestone Jul 29, 2019
@mairaw mairaw added vendor-task vendor-project Indicates the issue/pr is related to a vendor project. and removed vendor-task labels Jul 29, 2019
@mairaw mairaw closed this Jul 30, 2019
@mairaw mairaw reopened this Jul 30, 2019
@mairaw
Copy link
Contributor

mairaw commented Jul 30, 2019

Indentation was slightly off on the rendered sample: Opened a new PR for that dotnet/samples#1123

You can check the results here: https://review.docs.microsoft.com/en-us/dotnet/api/system.collections.generic.list-1.capacity?view=netframework-4.8&branch=pr-en-us-2838

Can you please address my comments here @WilliamAntonRohm?

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- thank you for correcting the indentation with your new PR. Going forward, I'll be sure the indentation is correct for both snippet tags & comment output.

@mairaw
Copy link
Contributor

mairaw commented Jul 30, 2019

I'll close and reopen this because the build seems stuck still. I can see the result on OPS but for some reason it's not showing up here.

@mairaw mairaw closed this Jul 30, 2019
@mairaw mairaw reopened this Jul 30, 2019
@mairaw mairaw changed the title Try. NET update for system.collections.generic.list-1.add Try. NET update for system.collections.generic.list-1.capacity Jul 30, 2019
@mairaw
Copy link
Contributor

mairaw commented Jul 30, 2019

The same snippet reference "~/samples/snippets/csharp/VS_Snippets_CLR/List`1_Class/cs/source.cs#1" is used throughout this file several times. You could add the note and the -interactive to all of those at once as well since the snippet was changed if you don't mind. You can do that in the new PR you're working on.

@mairaw
Copy link
Contributor

mairaw commented Jul 30, 2019

And I've changed the PR title to indicate the right API name where this is being added.

@WilliamAntonRohm
Copy link
Contributor Author

WilliamAntonRohm commented Jul 30, 2019

I'll update those snippet reference instances & add the corresponding notes -- thanks for updating the title!

@mairaw
Copy link
Contributor

mairaw commented Jul 31, 2019

On this same PR @WilliamAntonRohm or on a new one?

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- in this same PR, as they're identical changes to the original single change. If you prefer, I'll sign-off on this PR (2838) and roll them into my new combined PR instead.

@WilliamAntonRohm
Copy link
Contributor Author

WilliamAntonRohm commented Aug 1, 2019

Updated remaining 8 instances of that sample snippet (for a total of 9).

#sign-off

@WilliamAntonRohm WilliamAntonRohm changed the title Try. NET update for system.collections.generic.list-1.capacity Try. NET update for system.collections.generic.list-1.* Aug 1, 2019
@mairaw
Copy link
Contributor

mairaw commented Aug 1, 2019

Reviewing now...

Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @WilliamAntonRohm. One tiny correction before we merge.

@WilliamAntonRohm
Copy link
Contributor Author

#sign-off

@mairaw
Copy link
Contributor

mairaw commented Aug 2, 2019

Thanks @WilliamAntonRohm. Feel free to open additional PRs so you don't have to keep waiting for our reviews to move forward.

@mairaw mairaw closed this Aug 2, 2019
@mairaw mairaw reopened this Aug 2, 2019
@mairaw
Copy link
Contributor

mairaw commented Aug 2, 2019

Closed accidentally 🤦‍♀

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- it seems you need to approve the most recent change (plural-to-singular) before this PR can be merged. Thank you.

@mairaw
Copy link
Contributor

mairaw commented Aug 2, 2019

I did already. I'll just wait for build to finish again to merge this.

@WilliamAntonRohm
Copy link
Contributor Author

WilliamAntonRohm commented Aug 2, 2019

@mairaw -- thank you for approving -- I was three minutes behind the times :^)

@WilliamAntonRohm
Copy link
Contributor Author

#sign-off

@mairaw mairaw merged commit 9581c38 into dotnet:master Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

vendor-project Indicates the issue/pr is related to a vendor project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants