Conversation
|
@0x5bfa what do you think? |
0x5bfa
left a comment
0x5bfa
left a comment
There was a problem hiding this comment.
I think some of them are default values and can be omitted but LGTM.
Looks good, once this merged we'd like to run "dotnet format -w PROJ_PATH" and create spell check exclude dictionary.
| dotnet_naming_symbols.constants.applicable_accessibilities = * | ||
| dotnet_naming_symbols.constants.required_prefix = | ||
| dotnet_naming_symbols.constants.required_suffix = | ||
| dotnet_naming_symbols.constants.required_capitalization = all_upper |
There was a problem hiding this comment.
I think Pascal is fine. Title case is used in C/C++
There was a problem hiding this comment.
Wait, we require this. The normal C# convention is to use PascalCase for constants, we should amend.
There was a problem hiding this comment.
Sorry, you mean use PascalCase here? I don't understand
The docs literally say use UPPER for constant variables.
There was a problem hiding this comment.
OMG I thought I sent my message. But yes we should amend the guidelines because that convention is mostly for C/C++ AFAIK (prob in other langs too)
| dotnet_naming_rule.camel_case_for_variables.style = camel_case | ||
| dotnet_naming_rule.camel_case_for_parameters.style = camel_case | ||
| dotnet_naming_rule.pascal_case_for_properties.style = pascal_case | ||
| dotnet_naming_rule.upper_case_for_constants.style = all_upper |
0x5bfa
added
changes requested
|
This has an issue: _naming_style.asyncsuffix.required_suffix = Async`Async event methods should not have Async suffix so this ruling is wrong. This PR introduces many default values too, I think we should introduce one by one as required. Fyi @yaira2 |
This is the pattern we've been using in Files. |
Afaik, async event methods don't have the suffix. #13567 |
30cea65 to
decf3da
Compare
|
Ultimately I will need to completely rewrite this editorconfig, but the process of creating one is tedious and requires being very familiar with the codebase styles. |
75d29b5 to
aa7d7fa
Compare
|
@Lamparter is it alright if I close the PR in the meantime? |
3 participants
Resolved / Related Issues
Steps used to test these changes
N/A
More details are coming soon!