Skip to content

updating Guid.xml samples for Try .NET #1654

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 31 commits into from
Oct 22, 2019
Merged

updating Guid.xml samples for Try .NET #1654

merged 31 commits into from
Oct 22, 2019

Conversation

WilliamAntonRohm
Copy link
Contributor

@WilliamAntonRohm WilliamAntonRohm commented Oct 18, 2019 •
edited
Loading

@BillWagner
Copy link
Member

@WilliamAntonRohm I reviewed all these changes. I generally agree with @Youssef1313 's comments. You can apply those suggestions, and then this is ready to :shipit:

WilliamAntonRohm and others added 9 commits October 21, 2019 15:18
@WilliamAntonRohm @Youssef1313
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.ctor/cs/cto…
2ce654b 
…r1.cs

Co-Authored-By: Youssef Victor <[email protected]>
@WilliamAntonRohm @Youssef1313
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.ctor/cs/cto…
c967211 
…r2.cs

Co-Authored-By: Youssef Victor <[email protected]>
@WilliamAntonRohm @Youssef1313
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.empty/cs/em…
38214c0 
…pty.cs

Co-Authored-By: Youssef Victor <[email protected]>
@WilliamAntonRohm @Youssef1313
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/pa…
c57ea4f 
…rseex1.cs

Co-Authored-By: Youssef Victor <[email protected]>
@WilliamAntonRohm @Youssef1313
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/pa…
d9799fb 
…rseexactex1.cs

Co-Authored-By: Youssef Victor <[email protected]>
@WilliamAntonRohm @Youssef1313
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/tr…
5e14ce1 
…yparseexactex1.cs

Co-Authored-By: Youssef Victor <[email protected]>
@WilliamAntonRohm @Youssef1313
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.tobytearray…
5f1ae2d 
…/cs/tobytearray3.cs

Co-Authored-By: Youssef Victor <[email protected]>
@WilliamAntonRohm @Youssef1313
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.tobytearray…
3e2739d 
…/cs/tobytearray3.cs

Co-Authored-By: Youssef Victor <[email protected]>
@WilliamAntonRohm @Youssef1313
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.tobytearray…
65bd20a 
…/cs/tobytearray3.cs

Co-Authored-By: Youssef Victor <[email protected]>
@WilliamAntonRohm
Copy link
Contributor Author

@Youssef1313 -- Youssef, thank you for your helpful suggestions. I have applied them and I'll look for future such strings.

@BillWagner -- Bill, please merge.

mairaw
mairaw reviewed Oct 22, 2019
View reviewed changes
mairaw
mairaw requested changes Oct 22, 2019
View reviewed changes
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. The changes look great. Since we've started making changes to use var and string interpolation, I thought we should be consistent.

@mairaw mairaw added this to the October 2019 milestone Oct 22, 2019
WilliamAntonRohm and others added 13 commits October 22, 2019 09:42
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.empty/cs/em…
5d83c7c 
…pty.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.empty/cs/em…
2ce696a 
…pty.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/tr…
db34025 
…yparseexactex1.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/tr…
73e37b9 
…yparseexactex1.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.tobytearray…
228f020 
…/cs/tobytearray3.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.tobytearray…
ff9a634 
…/cs/tobytearray3.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.tobytearray…
b3081ad 
…/cs/tobytearray3.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.newguid/cs/…
d06ec1f 
…ng.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.newguid/cs/…
d7c3bb5 
…ng.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/pa…
9ca7a03 
…rseex1.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/pa…
7557781 
…rseexactex1.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/tr…
08cd71d 
…yparseex1.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@WilliamAntonRohm @mairaw
Update snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/tr…
88d15a8 
…yparseexactex1.cs

Co-Authored-By: Maira Wenzel <[email protected]>
@mairaw
Copy link
Contributor

mairaw commented Oct 22, 2019

You can apply all suggestions in bulk using the Files changed tab (https://github.com/dotnet/samples/pull/1654/files).

@mairaw
Copy link
Contributor

mairaw commented Oct 22, 2019

Only the manual suggestions left and then we can merge

mairaw
mairaw approved these changes Oct 22, 2019
View reviewed changes
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.

Thank you @WilliamAntonRohm!

@WilliamAntonRohm
Copy link
Contributor Author

@mairaw -- Maira, thanks for that tip! I believe these samples are all ready to go now.

@mairaw mairaw merged commit 7b12611 into dotnet:master Oct 22, 2019
@WilliamAntonRohm WilliamAntonRohm deleted the guid branch October 22, 2019 17:46
Youssef1313
Youssef1313 reviewed Oct 22, 2019
View reviewed changes
snippets/csharp/VS_Snippets_CLR_System/system.guid.parse/cs/tryparseex1.cs
foreach (var stringGuid in stringGuids) {
if (Guid.TryParse(stringGuid, out newGuid))
Console.WriteLine("Converted {0} to a Guid", stringGuid);
if (Guid.TryParse(stringGuid, out var newGuid))
Copy link
Member

Choose a reason for hiding this comment

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

I think this change is incorrect. var is used when declaring variables. However, I still didn't test this in an IDE.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did test this on https://try.dot.net/. Check this: https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-7#out-variables

I'd use discards since that variable is not used but try.net didn't like that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also seeing some occurrences of var which would compile and be correct C# code, but doesn't follow the guidelines, which says to use var when the type is clear from the right hand side.

Copy link
Member

Choose a reason for hiding this comment

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

@mairaw, Thanks, didn't know this feature of C# 7.

I'll open another PR for the other occurrences of var that doesn't follow the guidelines like:

var g = Guid.NewGuid();

Copy link
Member

Choose a reason for hiding this comment

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

@Youssef1313

var g = Guid.NewGuid();

To me, that example is clear. I would be be very surprised if a method Guid.NewGuid() returned anything other than a Guid. When I reviewed the changes in this PR, I didn't find uses of var that were unclear. I don't think another PR is needed here.

@WilliamAntonRohm WilliamAntonRohm mentioned this pull request Oct 22, 2019
Merged
@BillWagner BillWagner mentioned this pull request Oct 23, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mairaw mairaw mairaw approved these changes

@Youssef1313 Youssef1313 Youssef1313 left review comments

@BillWagner BillWagner Awaiting requested review from BillWagner

Assignees

@WilliamAntonRohm WilliamAntonRohm

Labels
None yet
Projects
None yet
Milestone
October 2019
Development

Successfully merging this pull request may close these issues.
4 participants
@WilliamAntonRohm @BillWagner @mairaw @Youssef1313