-
-
Notifications
You must be signed in to change notification settings - Fork 172
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
Improved .editorconfig compliance with .NET guidelines #3
Comments
How Naming Convention rules are parsed from a single fileFrom https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-naming-conventions:
From https://editorconfig.org/#file-location:
https://editorconfig.org/#file-format-details
Thus, both of the following are true:
See also https://github.com/MicrosoftDocs/visualstudio-docs/issues/1351, where I ask the official sources about related questions in the multi-file scenario. |
The pull request, and more specifically the table at 9b7d002 starting at line 282, is informative. This table lists:
|
This is a pretty big change that I need time to digest and test against the |
Hi @RehanSaeed, Have you taken a look at this change yet? I specifically created the table summarizing the differences between old and new rule sets, and included links to why it appears the updated rule is more appropriate. Let me know... |
Sorry it took so long to look into this. I tried out some fields and this is what I got:
This is what is valid in the current implementation:
|
The .NET guidelines state:
That page provides strong justification for the rules, mostly related to things breaking encapsulation and therefore causing maintenance nightmares when needing to change something in a later revision. In fact, apart from a fix to private fields being noisy, I think your investigation shows that the rules I proposed are working as exactly as intended, and in fact highlight significant encapsulation concerns. Here's a breakdown of the differences you noted:
The proposed rules are correct. The use of a protected field is disallowed, as it breaks encapsulation and causes versioning issues.
Walking through the rules, neither group A nor B should match, as they each require "public". Group "L" should be the first match, and mark it as INVALID, but you wrote the comment "Field should be private". It appears to me that the proposed rules are correct, because the use of a protected field is disallowed, even if marked as "internal". Are you suggesting the rule needs a fix of some sort here?
The proposed rules are correct. Public fields are disallowed unless either (1) public const, or (2) public static readonly. Neither of these lines fits these exceptions (readonly != static readonly).
OK, found the fix. I just need to set group L to "Silent". I submitted a new pull request (#5). This pull request will now allow you to name your private fields however you like, without emitting any warning. @RehanSaeed, would appreciate your thoughts, as with the "silent" fix, all the differences you noted seem to be properly flagged as lurking problems (at least according to the .NET guidelines). |
Still lots of work to do. Here I have added const, static readonly, static, readonly, and normal fields but have also added uppercase and lowercase variants of each. I have added comments against each with what is actually happening and then what I think should be happening.
|
@RehanSaeed, it seems I was solving a different problem. The problem I had been solving was to only include the rules from the .NET guidelines. In contrast, your comments suggest that you also require the .editorconfig to have rules for private fields. Not a problem! I wrote the rules specifically to allow such flexibility, and even had comments showing where those new rules should exist. But, as a proper set of naming rules is tricky, I also went ahead and generated the rules that you appear to be asking for.... with the exception that protected fields are still not allowed. Please check out pull request #5. |
Yes, I see. I think it makes sense to make the rules comprehensive. The .NET Guidelines leave a lot of room for interpretation. Getting closer. Not quite there yet:
|
@RehanSaeed , I think many of the things you indicate as "should be valid" are not, actually, valid under the .NET Guidelines. Let me address these in three groups: Each is posted below (hyperlinks included above). Each post ends with a question for you. |
Group 1 -- .NET GuidelinesThe rules applicable to this group:
The .NET Guidelines provide two exceptions to public fields:
These are covered in the rules as follows:
You suggest the items in this first group should be valid. However, they are each public fields, and thus the .NET Guidelines prohibit them, because they do not fall into one of the two enumerated exceptions.
In view of the above, do you agree that the rules I provided accurately mark each of the above as invalid? If not, can you point to the rules that indicate these should be allowed / valid / PascalCase? |
Group 2 -- new C# 7.2 support for private protectedYou correctly ask what the docs (.NET Guidelines) say. The .NET Guidelines were written prior to this combination of permissions being defined, and as thus (technically) silent. However, the .NET Guidelines explain their rationale.
The definition of private protected is:
Based on the above definition, it is clear that the Therefore, the following two items are NOT defined by the .NET Guideline rules for fields (A/B, L/M). However, I DID include them in your private rule sets (C/D), and will address them in the third post.
Do you agree with the above analysis, and thus believe the .NET Guidelines would not directly apply to this field type? If not, can you help me understand the alternatives (and preferably, support for the alternative)? |
Group 3 -- Additional / extended rulesPresuming you agree with the above two posts, we have only a few items remaining. Each of these falls outside the .NET Guidelines, but generally matches your prior-stated desires, which I will summarize here in the order the rules are applied. Note: These rules do not apply to public fields, which are matched by Rulesets A/B.
Again, these are long, so I will break into three posts: |
Group 3a - New C# 7.2
|
Group 3b - Internal fieldsYou previously wrote:
Based on the above comments, I understood that you desired both However, just now, you indicated that each of the below should be found as INVALID:
I am now at a loss as to what your expectations are for Can you help me understand what rules you expect for |
Group 3c - Fully private fieldsPer your earlier comments, you wrote only the following:
Now, you write the following:
Based on the above, is it accurate to state that you want to enforce |
Group 1 -- .NET GuidelinesI think you are correct. So we end up with the following expected result:
|
Group 2 -- new C# 7.2 support for private protectedI suspect we should treat |
Group 3a - New C# 7.2 private protected fieldsAs I've said above, I suspect the best thing to do is treat |
Group 3b - Internal fieldsGiven this:
I think internal also counts here. So therefore all of this would be
|
Group 3c - Fully private fields
Yes, that goes along with the guidelines does it not? |
I think it's most useful to look at the test cases below. If I'm right, based on all your comments this is what we're now aiming for:
This is so complex. Apologies if this is taking too long. Give me a shout on Teams and we can talk through any differences in our understanding. GitHub comments are not a great forum for this kind of problem. |
@RehanSaeed I'd already created a GIST that documents each option. The comments start with "good" if the rule matches expectations, followed by parenthesis indicated the expected results, followed by an indication of which Ruleset causes that result. You can load that GIST into a solution and compile. It seems we're down to two questions:
Differentiating between what the .NET Guidelines require, and what they leave to the user's preferences, is a key part of this Issue and Pull Request. Group 2 / 3a / 3bFor the purposes of the .NET Naming Guidelines, There are many valid reasons to allow internal fields and use internal fields. Any rule in .editorconfig dealing with them would be user-specific. You previously indicated that internal fields would be PascalCasing ('must be uppercase'). I agree that is the right result, and that's what the rules currently apply.... |
(Summarizing out-of-band conversation with @RehanSaeed)
I'll ping again with an updated pull request. |
@RehanSaeed Pull request updated. |
A sample C# project was a good idea, so kudos. A few minor changes requested. Also added you as a core contributor. |
This is a somewhat long post, expanding somewhat on Dotnet-Boxed/Framework#22.
TLDR;
Foundational Background
Protected fields should be treated as public, according to .NET design guidelines.
The same .NET design guidelines drastically limit allowed public fields, by allowing only two exceptions.
The naming convention guidelines explicitly exclude internal and private fields. Thus, even if allowed, these are entirely user preference.
Because of the above, the state for a given field is defined by:
{true|false}
field type allowed by the .NET design guidelines{null|pascal}
naming convention specified by the naming guidelinesThe recommendation is to split field name formatting as follows:
{true, pascal}
== public const, protected const{true, pascal}
== public static readonly, protected static readonly{true, null}
== catch-all for private, internal, and protected internal fields{false, *}
== any other type of field not in above categoriesThe text was updated successfully, but these errors were encountered: