-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Updated naming pattern used the s_ prefix for private static fields #56647
base: main
Are you sure you want to change the base?
Updated naming pattern used the s_ prefix for private static fields #56647
Conversation
@siyandand1 thanks for contacting us. This PR doesn't seem to have any issue associated with it. Please file an issue and start the discussion in the issue before making any changes. We aren't going to take changes like this without a discussion and without the team agreeing that this is the right thing to do and important to do moving forwards. |
@JamesNK can you please review this? Thanks! |
@mkArtakMSFT before any review happens, I think this needs discussion with the broader team. |
Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at. |
@JamesNK pointed this out in the team chat. Conceptually I don't have a problem with changing our default to align with dotnet/runtime. I think we just need to beware any unexpected consequences:
|
@mitchdenny I think this is a style option and we shouldn't be imposing a convention without broader discussion across the team. (Maybe through email or DoI (@wtgodbe)). This is not a convention our team follows, and I think that's fine. It's weird that we would choose to adopt something like Our convention is essentially I don't want to turn this into a discussion, but I think it deserves more talking before we go in one direction as this break's years of muscle memory and doesn't offer any benefit beyond what |
My two cents: I think resolving the inconsistency between PascalCase and underscore case for private static fields in the codebase is a net good. In that case, I would propose moving from PascalCase for some fields to underscore case for all fields. IMO, that solves the immediate problem of inconsistency between the two and avoids introducing a new set of issues (around member and static fields, like @javiercn mentioned above). With that being said, it would be good to evaluate the size of the code delta associated with this change. As @mitchdenny mentioned, it'll introduce a pain around backporting. Also, I'm cautious about merging this change during the preview7/RC1 timelines which tend to be high traffic with respect to merged PRs and having to solve merge conflicts related to the rename might prove tedious. |
This change (approx 400 lines) isn't large. I don't think complicating backports should be a concern. And this change is minor compared to adding nullable annotations everywhere or switching to file scoped namespaces. @javiercn In case you missed it, I started an informal poll in the aspnetcore team chat. |
@@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Components; | |||
|
|||
internal sealed class ElementReferenceJsonConverter : JsonConverter<ElementReference> | |||
{ | |||
private static readonly JsonEncodedText IdProperty = JsonEncodedText.Encode("__internalId"); | |||
private static readonly JsonEncodedText s_IdProperty = JsonEncodedText.Encode("__internalId"); |
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.
This should be s_idProperty
. The character after the first underscore should be lower case.
Applies to all fields.
Talked with team members offline and we'll go with |
Can you add this rule to the .editorconfig file in the root of the respoitory: To the .editorconfig file in the root of the respoitory: https://github.com/dotnet/aspnetcore/blob/7498cea189f5e32a11ef480729384ff6b5dc662f/.editorconfig Try to be consistent with the location so the editorconfigs are similar. The rule will enforce the style with an analyzer. |
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
@siyandand1 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by .NET Foundation and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant .NET Foundation, and those who receive the Submission directly b. Patent License. You grant .NET Foundation, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to .NET Foundation. You agree to notify .NET Foundation in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and .NET Foundation dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
Title: Enforce Consistent Naming for Private Static Fields
Description:
The dotnet/aspnetcore repository currently mixes underscore and PascalCase naming conventions for private static fields. To ensure code consistency and readability, we should adopt a single naming convention. Given that dotnet/runtime uses the s prefix for private static fields, I propose we align with this convention.
Changes Proposed:
Rationale:
Fixes #50888
Consistency in naming conventions helps maintain readable and maintainable code. By aligning with the dotnet/runtime convention, we ensure uniformity across the broader .NET ecosystem.
Changes Made: