[System.Threading.Thread] Follow code style

[lambdageek]

followup to #68232

[ghost]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

---

followup to #68232

Author:	lambdageek
Assignees:	lambdageek
Labels:	area-System.Threading
Milestone:	-

[ShreyasJejurkar]

Maybe we can put this rule in the global editor config!? And maybe severity to warning?

Rule link => https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/formatting-rules#csharp_new_line_before_open_brace

[Edited]
The rule is already there, but it didn't warn? https://github.com/dotnet/runtime/blob/main/.editorconfig#L22

[lambdageek]

[Edited] The rule is already there, but it didn't warn? https://github.com/dotnet/runtime/blob/main/.editorconfig#L22

I don't use an editor that reads .editorconfig, or that displays compilation warnings.

But I'll try to build with warnings as errors next time I modify c# files.

[ShreyasJejurkar]

[Edited] The rule is already there, but it didn't warn? https://github.com/dotnet/runtime/blob/main/.editorconfig#L22

I don't use an editor that reads .editorconfig, or that displays compilation warnings.

But I'll try to build with warnings as errors next time I modify c# files.

No no you got me wrong 😅, you can use the editor whatever you like. My point was the config is already there in place, why CI didn't fail when the rule was violated? Maybe the value is just set to all instead of all:warning ?

cc @stephentoub

[stephentoub]

why CI didn't fail when the rule was violated?

Because we don't enforce every piece of guidance on style preference in the editorconfig; it is a preference, not a requirement.

[ShreyasJejurkar]

why CI didn't fail when the rule was violated?

Because we don't enforce every piece of guidance on style preference in the editorconfig; it is a preference, not a requirement.

umm ok. Got that. But shouldn't we follow a consistent style across the codebase? And warn users if they didn't follow?

[stephentoub]

But shouldn't we follow a consistent style across the codebase?

If you're starting from scratch with a greenfield app, sure. If you've got millions and millions of lines of code written by hundreds of developers over two decades following disparate guidelines for style, forcing every line of existing code to adhere to every one of the style preferences is not a high priority.

[ShreyasJejurkar]

Yessss, totally agree!

On the other side, how I think about this one, is like if someone finds a violation we should create an issue on the repo with a good-first-issue label and explain there what needs to be done. That way we can fix violations (slowly-slowly) and can increase community contributions as well! Thoughts!?

[stephentoub]

That way we can fix violations (slowly-slowly) and can increase community contributions as well!

Contributions that just move a brace from one line to the next aren't particularly interesting to us; we even ask that PRs just about style not be submitted:
https://github.com/dotnet/runtime/blob/main/CONTRIBUTING.md#dos-and-donts
"DON'T make PRs for style changes."

By rough search, there are ~34,000 cases of a { in the "wrong" place. It's not a good use of anyone's time to proactively move those to their own line nor review PRs to do so, nor a good use of our CI resources to validate such changes.

[ShreyasJejurkar]

Hmm yeah. ~34,000 cases surely look like a lot of work.

Thanks, Stephen for the clarification, appreciate it a lot! There is always a thing that I learn every single time I get a chance to chat with you! 😅🙌

[lambdageek]

Looks like there are other follow-up changes that I need to land for wasm-mt. Closing this in favor of #68563