Skip to content
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

Update the default .NET editorconfig template #4598

Closed
wants to merge 1 commit into from

Conversation

sliekens
Copy link

Problem

The template generated by dotnet new editorconfig has not been updated in a while. VS2022 generates a newer template, which is also documented here.

Solution

I copied the template from the link above.

Checks:

  • Added unit tests (surely can't be relevant?)
  • Added #nullable enable to all the modified files ? (again not relevant?)

@sliekens sliekens requested a review from a team as a code owner April 15, 2022 16:12
# Xml files
[*.xml]
# XML project files
[*.{csproj,vbproj,vcxproj,vcxproj.filters,proj,projitems,shproj}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lacks support for F# project files, fsproj.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch but do you want to change it here when the template in VS2022 has the same problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's more a note that we should resolve it in both places. Fixing it upstream in VS2022 first, then flowing down to here works for me. Where would that be reported?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baronfel file on roslyn please

@vlada-shubina vlada-shubina requested a review from jmarolf April 27, 2022 07:40
@vlada-shubina
Copy link
Member

@jmarolf could you please check it and advise if we should take this change? thank you.
I checked it and proposed changes matching the editorconfig created by VS 2022 for C# project, however it's a lot of changes so want to double-check if it is as expected. Thank you.

@jmarolf jmarolf self-assigned this Apr 27, 2022
@jmarolf
Copy link
Contributor

jmarolf commented Apr 27, 2022

Thanks @vlada-shubina! I've assigned to myself and will go through the changes.

@agilenut
Copy link

I'd be curious to know why the following were removed from the VS output:

  • dotnet_separate_import_directive_groups = true (not found)
  • file_header_template = unset (found - default)
  • dotnet_style_prefer_compound_assignment = true:suggestion (found - default)
  • dotnet_style_prefer_simplified_boolean_expressions = true:suggestion (found - default)
  • dotnet_style_prefer_simplified_interpolation = true:suggestion (found - default)
  • dotnet_code_quality_unused_parameters = all:suggestion (found - default)
  • dotnet_remove_unnecessary_suppression_exclusions = none (found - default)
  • csharp_style_expression_bodied_lambdas = true:suggestion (found - default)
  • csharp_prefer_static_local_function = true:warning (found - true:suggestion is default)
  • csharp_prefer_simple_using_statement = true:suggestion (found - default)
  • csharp_style_prefer_index_operator = true:suggestion (found - default)
  • csharp_style_prefer_range_operator = true:suggestion (found - default)
  • csharp_style_throw_expression = true:suggestion (found - default)
  • csharp_style_unused_value_assignment_preference = discard_variable:suggestion (found - default)
  • csharp_style_unused_value_expression_statement_preference = discard_variable:silent (found - default)
  • csharp_using_directive_placement = outside_namespace:silent (found - default)
  • csharp_indent_block_contents = true (not found)
  • csharp_indent_braces = false (not found)
  • csharp_indent_case_contents_when_block = true (not found)
  • csharp_space_after_comma = true (not found)
  • csharp_space_after_dot = false (not found)
  • csharp_space_after_semicolon_in_for_statement = true (not found)
  • csharp_space_around_declaration_statements = false (not found)
  • csharp_space_before_comma = false (not found)
  • csharp_space_before_dot = false (not found)
  • csharp_space_before_open_square_brackets = false (not found)
  • csharp_space_before_semicolon_in_for_statement = false (not found)
  • csharp_space_between_empty_square_brackets = false (not found)
  • csharp_space_between_square_brackets = false (not found)

I checked each removed item against the documentation. I noticed that some of the items are not found and some were found. I've noted that in the parentheses.

@agilenut
Copy link

It would be nice if the upstream version from VS and this one included the following which is in a lot of dotnet repos like dotnet/runtime.

# Shell scripts
[*.sh]
end_of_line = lf
[*.{cmd,bat}]
end_of_line = crlf

#### Naming styles ####
[*.{cs,vb}]

# Naming rules

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing all of the name rules seems to be a fairly large loss. I also see that the name rules have changed in the VS output to only specify pascal cased constant fields but this seems less helpful. Furthermore, when I tested the editorconfig produced by VS last night, that naming rule didn't seem to work for me. The ones currently in the dotnet new template seem more complete and work correctly in VS2022.

@cremor
Copy link

cremor commented Apr 28, 2022

#4393 mentions a few EditorConfig rules that are currently missing in the file. It seems like they would still be missing with this PR. Please include them so the file is complete!

I think it would be better to use the EditorConfig file exported by the current version of Visual Studio 2022 (via Tools - Options - Text Editor - C# - Code Style - Generate .editorconfig file from settings) as a source.

@sliekens
Copy link
Author

sliekens commented Jun 1, 2022

via Tools - Options - Text Editor - C# - Code Style - Generate .editorconfig file from settings

You mean on a blank install of VS2022? It's starting to look like I opened a can of worms here.

@cremor
Copy link

cremor commented Jun 1, 2022

Shouldn't matter if it's a blank install or already modified. The exported file always contains all rules, just the rule value differs based on your VS settings.

@agilenut
Copy link

agilenut commented Jun 1, 2022

Shouldn't matter if it's a blank install or already modified. The exported file always contains all rules, just the rule value differs based on your VS settings.

I can see why someone would want this behavior. However, I would think having the dotnet CLI templates somehow rely on VS settings would create a dependency issue. I'm assuming the CLI templates are not supposed to have knowledge about Visual Studio.

Also, part of the benefit of having a template like this is getting a consistent output with good defaults. If you had teams instructed to create an editorconfig with this template on new repos, they'd now be possibly creating different versions based on their own VS settings. This would lead to a level of inconsistency that could be considered a negative in a lot of organizations.

@cremor
Copy link

cremor commented Jun 1, 2022

I didn't mean that the dotnet CLI should use the current VS settings. Just that this pull request should contain all editorconfig rules that are understood by VS - which can be compared to the file which can be exported from VS.


# this. and Me. preferences
dotnet_style_qualification_for_event = false:silent
# this. preferences
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use this. and Me. preferences wording as this applied to both C# an VB

dotnet_sort_system_directives_first = true
file_header_template = unset
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this deleted?

# Organize usings
dotnet_separate_import_directive_groups = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this deleted?

Copy link
Contributor

@jmarolf jmarolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like the premise of this PR is that the example editorconfig here is somehow complete or desirable. Instead I think that doc page should be updated to show the new rules. this PR removes a lot of rules and changes formatting that I don't see the benefit of.

I would approve this if the changes were purely additive

@agilenut
Copy link

agilenut commented Jun 1, 2022

It sounds like the premise of this PR is that the example editorconfig here is somehow complete or desirable. Instead I think that doc page should be updated to show the new rules. this PR removes a lot of rules and changes formatting that I don't see the benefit of.

I would approve this if the changes were purely additive

Agreed.

Personally, I think the goals should be:

  1. Create a .editorconfig that has all of the current options. A partial list allows for individual developer settings to create inconsistency. It would only make sense to remove options if they have been deprecated or superseded by other options.
  2. Use values that most closely align with the Microsoft naming and style guidelines and that are used by the majority of the community (good defaults).
  3. Create consistency between editorconfigs produced by the various supported methods, including Visual Studio.

Unfortunately, it seems the changes in Visual Studio is driving this PR and they have made some non-ideal choices. It would be great if this could be addressed there as well.

Finally, I've noticed gaps in the documentation and I'm not sure if that is because the options have been deprecated or superseded or the documentation is just missing. It would be nice to address that in a separate effort.

@sliekens
Copy link
Author

sliekens commented Jun 1, 2022

It sounds like the premise of this PR is that the example editorconfig here is somehow complete or desirable.

Actually I'm proposing that the CLI should generate the same editorconfig as VS2022 when doing File | New
image

I cannot answer why some rules were removed, I don't know where to find the version history for VS2022 file templates.

I was unaware there is another way to generate an editorconfig file from the options menu so that adds another dimension to this issue and I'm considering abandoning this PR because I did not expect this to become so complicated.

@vlada-shubina vlada-shubina added the area: template-content The issue is related to content of template packages managed in this repo (/template_feed) label Jun 23, 2022
@vlada-shubina
Copy link
Member

Closing due to inactivity

@agilenut
Copy link

agilenut commented Aug 1, 2022

I've been following this hoping for some updates to the template. But this was just closed. What's the plan?

@sliekens
Copy link
Author

sliekens commented Aug 1, 2022

I don't see any open conversation between people who maintain this repo, people who maintain VS templates and we the people who are supposed to use both. So I think that would be a good start.

@agilenut
Copy link

agilenut commented Aug 1, 2022

I agree. I'd like t see that as well. Can someone maintaining this repo comment on the progress of reaching out to the VS team? Or, if there is another git repo that I can open an issue in to ask some questions, then I'd be happy to do that if someone can point me in the right direction.

Alternatively, if we want to update the editorconfig template in this repo to just be more current without worrying about all the strange changes that VS made, I'd be willing to create a slightly different PR.

@vlada-shubina
Copy link
Member

There are several reasons contributing to this PR being closed:

  • the templates are being moved to dotnet/sdk repo, so this PR becomes outdated and needs to be reopened in another repo.
  • author's comment stating that they are considering to abandon the PR
  • no activity for quite a while

If you'd have any specific questions\suggestions:

  • for .NET SDK template, please open the issue in dotnet/sdk or dotnet/templating repo. Once the templates are moved we will move the issues as well. From @jmarolf's review I see no approve on removing the content for .NET SDK template.
  • for VS template, the most appropriate way is to create VS feedback ticket

@jmarolf @baronfel please advise the better location if any or is there any plans to change .NET SDK editorconfig template.

@agilenut
Copy link

agilenut commented Aug 1, 2022

Thanks @vlada-shubina.

I'll raise an issue using the VS feedback ticket link you sent.

Do you have a timeline for when you expect the templates to be moved?

I saw that @jmarolf said that he'd approve this PR if it was purely additive. This is similar to my thoughts and what I'd create if I were make a different PR.

@cremor
Copy link

cremor commented Aug 1, 2022

@vlada-shubina

  • for .NET SDK template, please open the issue in dotnet/sdk or dotnet/templating repo. Once the templates are moved we will move the issues as well. From @jmarolf's review I see no approve on removing the content for .NET SDK template.

I've created #4393 some time ago so that the SDK template is updated. But it was closed in favor of the much more complex #4126

@agilenut
Copy link

agilenut commented Aug 1, 2022

VS Feedback posted: https://developercommunity.visualstudio.com/t/Changes-in-2022-removed-EditorConfig-set/10108852

@vlada-shubina
Copy link
Member

Thanks @vlada-shubina.

I'll raise an issue using the VS feedback ticket link you sent.

Do you have a timeline for when you expect the templates to be moved?

I saw that @jmarolf said that he'd approve this PR if it was purely additive. This is similar to my thoughts and what I'd create if I were make a different PR.

The templates should be moved by the end of this week, so i would rather wait until its done.
Please note that this change will likely appear only in .NET SDK 7.0.200.

I've created #4393 some time ago so that the SDK template is updated. But it was closed in favor of the much more complex #4126

Unfortunately, solution from #4126 is not longer possible for editorconfig as its content is being generated programmatically based on user settings. We will discuss other possible ways to keep content in sync in the following weeks and will create the issue with proposed solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: template-content The issue is related to content of template packages managed in this repo (/template_feed)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants