Skip to content

Is it a useful change, to replace built-in types with keywords? #6917

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

Merged
merged 4 commits into from
Aug 4, 2021

Conversation

WhiteBlackGoose
Copy link
Contributor

I'm not spamming now. I'm trying to learn what's a good change and what is not worth reviewing. I didn't make this change before, so I need to know if I should make it. So far I know what I can do:

  1. String formatting -> string interpolation (where appropriate)
  2. Convert.To -> casting/parsing etc. (where appropriate)

Please tell me if I should ping someone to review my PRs. Also, if this gets merged silently, would it be fair to consider this as "yes" to my initial question?

@WhiteBlackGoose WhiteBlackGoose requested a review from a team as a code owner July 23, 2021 16:37
@ghost
Copy link

ghost commented Jul 23, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@opbld30
Copy link

opbld30 commented Jul 23, 2021

Docs Build status updates of commit 0bd5b9b:

✅ Validation status: passed

File Status Preview URL Details
samples/snippets/csharp/VS_Snippets_CLR_System/System.AppContext.Class/cs/Example4.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/System.AppContext.Class/cs/Example6.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/System.AppContext.Class/cs/Example8.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/System.AppContext.Class/cs/GetSwitches3.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action_PrintExample/cs/action.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~3/cs/Action3.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~3/cs/Anon.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~3/cs/Delegate.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~3/cs/Lambda.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~4/cs/Action4.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~4/cs/Anon.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~4/cs/Delegate.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~4/cs/Lambda.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.activator.createinstance/cs/CreateInstance5.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.activator.createinstance/cs/createinstance2.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.activator.createinstance/cs/createinstanceex1a.cs ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@opbld34
Copy link

opbld34 commented Jul 23, 2021

Docs Build status updates of commit 85436b2:

✅ Validation status: passed

File Status Preview URL Details
samples/snippets/csharp/VS_Snippets_CLR_System/System.AppContext.Class/cs/Example4.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/System.AppContext.Class/cs/Example6.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/System.AppContext.Class/cs/Example8.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/System.AppContext.Class/cs/GetSwitches3.cs ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action_PrintExample/cs/action.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~3/cs/Action3.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~3/cs/Anon.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~3/cs/Delegate.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~3/cs/Lambda.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~4/cs/Action4.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~4/cs/Anon.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~4/cs/Delegate.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.Action~4/cs/Lambda.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.activator.createinstance/cs/CreateInstance5.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.activator.createinstance/cs/createinstance2.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR_System/system.activator.createinstance/cs/createinstanceex1a.cs ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@MSDN-WhiteKnight
Copy link
Contributor

It's best to open issue first if you want to submit a set of changes over multiple files or when you are not sure that changes will be accepted. See contributing guide: https://docs.microsoft.com/en-us/contribute/dotnet/dotnet-contribute

DON'T surprise us with large pull requests. Instead, file an issue and start a discussion so we can agree on a direction before you invest a large amount of time.

As for these changes, replacing type identifiers with language keywords is good, it was discussed recently here: dotnet/docs#24985 The system automatically pings the docs team, so your PR will be reviewed soon.

@WhiteBlackGoose
Copy link
Contributor Author

DON'T surprise us with large pull requests. Instead, file an issue and start a discussion so we can agree on a direction before you invest a large amount of time.

That's exactly why I did NOT open a big PR. So this PR is more of a question, but phrased as a PR :). The problem is that this change alone might take tens or hundreds of PRs, so I'm a bit worried that you don't want it.

It's best to open issue first if you want to submit a set of changes over multiple files or when you are not sure that changes will be accepted.

Right, but opening a PR immediately shows what I'm up to. I'm still a bit unsure whether it's good idea to fix so many things

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @WhiteBlackGoose

I've reviewed these changes, and I'll :shipit: now.

@BillWagner BillWagner merged commit 030c227 into dotnet:main Aug 4, 2021
@BillWagner
Copy link
Member

Thanks for the conversation @WhiteBlackGoose and @MSDN-WhiteKnight

This is a good way to start, and over time, improving our samples is a goal.

We'll continue to watch for and merge PRs that are roughly this size for these updates that bring our samples in line with our current stated coding standard.

That said, reviews and merging may occasionally be delayed depending on release cycles (including preview releases). Thanks for understanding.

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.

5 participants