Fix strings with s uppercase #426
Conversation
There was a problem hiding this comment.
Overall, this is great @lucasnobredev Thanks for continuing to contribute to docs.
I did make one suggestion because we should update that sample in a different way to address an issue. Once you update that, I'll merge this PR.
| Console.WriteLine("Its use could result in a NullReferenceException"); | ||
| } | ||
| else if (obj is var _) | ||
| else |
There was a problem hiding this comment.
Let's leave this sample as is for the moment. Issue dotnet/docs#8727 points to the need for a better sample. I'd rather leave this in place until we make a new sample that shows the usage better.
There was a problem hiding this comment.
Thanks, @lucasnobredev. The final change in this PR requires a change in the dotnet/docs repo, though. Could you open a PR to do that? I've requested changes here so that the two PRs can be merged at the same time.
| Console.WriteLine("Its use could result in a NullReferenceException"); | ||
| } | ||
| else if (obj is var _) | ||
| else |
There was a problem hiding this comment.
Although this change is acceptable, it requires a change to example description. The example is used in Discards - C# Guide, where the sentence "It also used the discard pattern to handle non-null objects of any other type." needs to be removed. Could you open a PR to do that?
|
@BillWagner and I reviewed this at about the same time, @lucasnobredev. I agree with him, though, that the final example should be left in place until we come up with a better example. Since you've also made the same update in #425, the last change can be reverted. |
|
@rpetrusha @BillWagner Done! Thank you, guys. |
|
@rpetrusha @BillWagner I was reading the article again and it seems like if we remove condition of code and remove the phrase "It also used the discard pattern to handle non-null objects of any other type", the section lost the sense to exist. I'll take this comment to dotnet/docs#8727 and I'll wait. |
|
I looked at the latest updates, and I'm ready to Let me know what you think @rpetrusha |
|
I agree that this is ready to merge, @BillWagner. Thanks, @lucasnobredev. I've tagged this as merge on hold for live update, since the merge to live build is still running. It can be merged once that build completes and the merge-to-live PR is closed. |
rpetrusha
added
verify-build-before-merge
4 participants
Summary
This PR is from dotnet/docs#8700.
Fixes dotnet/docs#8700