-
Notifications
You must be signed in to change notification settings - Fork 6k
Post merge review and customer feedback #24237
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
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm,... even the very basic example code in C# reference docs does not follow the newly added convention
s_
. It's not so popular outside runtime repo.https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/static#example---static-initialization
I think it's too late to add these new guidelines after 20 years have passed since C# was born.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency in official docs is so important. It needs a lot of work to correct these examples. Is it really worth to change at this time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @SIkebe,
It's never too late to update guidelines. It is natural for them to morph over time - especially with a language that continues to evolve. Following the .NET runtime as a basis is a solid foundation, since it's open source and in the view of the public. Even within the runtime code base, you can find violations and inconsistencies - that's okay. There are bound to be inconsistencies, but those too (when and where applicable) could be addressed. Guidelines are a set of suggestions, not something that is mandated. You're free to ignore them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree if there is a decent need to do so, or strong feedback from users. Could you point to the discussion about this newly added convention? Users want to know "why" this happened at this time.
And how about @nanasi880 's point?
#24054 (comment)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IEvangelist
@SIkebe
I agree with this.
It's not that I don't like the change, but the reason for the change is unclear, and as I pointed out, this is a change that Microsoft has explicitly denied.
It seems that static variables have an 's_' prefix and thread-static variables have 't_', what about AsyncLocal for example, and ThreadLocal?
Also, what if there are more of these in the future?
What if fiber is born? Is 'f_' the prefix that fiber local storage has?
We believe that history has proven that distinguishing these things by prepositions is a bad idea.
This is exactly what we call Hungarian notation, and we don't think it's an idea we should adopt, at least not actively now.