editorconfig codestyle options #15029

Merged
merged 4 commits into from Nov 17, 2016

Projects

None yet

7 participants

@jmarolf
Member
jmarolf commented Nov 5, 2016

@dotnet/roslyn-ide @jasonmalinowski
Adds the following options

Code Style Options

editorconfig name possible values
csharp_style_var_for_locals true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_var_for_built_in_types true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_var_when_type_is_apparent true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_var_elsewhere true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_expression_bodied_methods true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_expression_bodied_constructors true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_expression_bodied_operators true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_expression_bodied_properties true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_expression_bodied_indexers true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_expression_bodied_accessors true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_pattern_matching_over_is_with_cast_check true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_pattern_matching_over_as_with_null_check true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_inlined_variable_declaration true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_throw_expression true: none,suggestion,warning,error , false: none,suggestion,warning,error
csharp_style_conditional_delegate_call true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_qualification_for_field true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_qualification_for_property true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_qualification_for_method true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_qualification_for_event true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_predefined_type_for_locals_parameters_members true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_predefined_type_for_member_access true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_object_initializer true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_collection_initializer true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_coalesce_expression true: none,suggestion,warning,error , false: none,suggestion,warning,error
dotnet_style_null_propagation true: none,suggestion,warning,error , false: none,suggestion,warning,error
@jmarolf jmarolf editorconfig codestyle options
b887870
@jmarolf
Member
jmarolf commented Nov 5, 2016

retest vsi please

@jmarolf jmarolf added the Area-IDE label Nov 7, 2016
@brettfo
brettfo approved these changes Nov 8, 2016 View changes
@@ -0,0 +1,29 @@
+namespace Microsoft.CodeAnalysis.CodeStyle
@brettfo
brettfo Nov 8, 2016 edited Member

Copyright. #Closed

@jmarolf jmarolf adding tests
3fba790
@jasonmalinowski
Member

@jmarolf Is this now subsumed by your other PR or does this still need a review?

@jmarolf
Member
jmarolf commented Nov 11, 2016

@jasonmalinowski #15065 depends on changes in this PR. I was thinking that that this would merge in before #15065 but I am going to take the feedback you left there and apply it here.

@jasonmalinowski

I'd move the parsing logic into the core .editorconfig logic since a CodeStyleOption is a core concept to us, just to save some verbosity. We did that in other persisters already.

+ bool isEnabled = false;
+ if (args.Length != 2)
+ {
+ if (arg.Length == 1)
@jasonmalinowski
jasonmalinowski Nov 11, 2016 Member

Not sure why we have nested ifs here...this seems a bit tricky.

@jmarolf
jmarolf Nov 14, 2016 Member

This handles the case where the user specified that a rule should be on but has not specified a severity, so we just use the default.


In reply to: 87656798 [](ancestors = 87656798)

+ return CodeStyleOption<bool>.Default;
+ }
+ }
+ return CodeStyleOption<bool>.Default;
@jasonmalinowski
jasonmalinowski Nov 11, 2016 Member

Is this right? Wouldn't we want to say that we should return the default for the option which might be different than this?

@jmarolf
jmarolf Nov 14, 2016 Member

The semantics for these editorconfig files is that if the user has specified the option, but we can't parse it, we pass the default option. Would you prefer we throw?


In reply to: 87657312 [](ancestors = 87657312)

+
+ switch (args[1].Trim())
+ {
+ case "none": return new CodeStyleOption<bool>(value: isEnabled, notification: NotificationOption.None);
@jasonmalinowski
jasonmalinowski Nov 11, 2016 Member

Still not sure I like "none" because it's confusing to me if that's the severity or the value. 'silent' perhaps?

@@ -20,55 +21,73 @@ public class CodeStyleOptions
/// This option says if we should simplify away the <see langword="this"/>. or <see langword="Me"/>. in field access expressions.
/// </summary>
public static readonly PerLanguageOption<CodeStyleOption<bool>> QualifyFieldAccess = new PerLanguageOption<CodeStyleOption<bool>>(nameof(CodeStyleOptions), nameof(QualifyFieldAccess), defaultValue: CodeStyleOption<bool>.Default,
- storageLocations: new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Specific.QualifyFieldAccess"));
+ storageLocations: new OptionStorageLocation[]{
+ new EditorConfigStorageLocation("dotnet_style_qualification_for_field", ParseEditorConfigCodeStyleOption),
@jasonmalinowski
jasonmalinowski Nov 11, 2016 Member

I'm not morally against teaching EditorConfigStorageLocation about CodeStyle options directly to avoid having to pass the parser to each one specifically. We did that in the RoamingProfileStorageLocation as well.

jmarolf added some commits Nov 14, 2016
@jmarolf jmarolf changing option to "silent" from "none" 3230a15
@jmarolf jmarolf teach EditorConfigStorageLocation about code style options
3d6680b
@jmarolf jmarolf changed the base branch to dotnet:dev15-rc2 from dotnet:master Nov 15, 2016
@jmarolf
Member
jmarolf commented Nov 15, 2016

@Pilchie @MattGertz @natidea I've re-targeted the PR against RC.2, please let me know it we want to take this.

Customer scenario

  • The customer can now set visual studio settings in editorconfig files that allow codestyle options to be specified per folder.

Bugs this fixes: (either VSO or GitHub links)

Workarounds

  • The user can manually change their code style settings each time they open a solution file, but cannot have different settings from different folders within that solution.

Risk

  • This uses the editorconfig API from the Visual Studio Platfrom team, it does not introduce code into roslyn other than consuming that API and handing it to the options service

Performance impact

  • When folders are quieried is set by the Visual Studio Platfrom team which has done the work to not requery unless their are file changes. Perf impact should be Low

Is this a regression from a previous update?

  • No.

Root cause analysis: how did we miss it? What tests are we adding to guard against it in the future?

  • This is not a regression. It is new feature work.

How was the bug found? (E.g. customer reported it vs. ad hoc testing)

  • This is not a bug. It is new feature work.
@jmarolf
Member
jmarolf commented Nov 17, 2016

Tests seemed to have stalled. Closing and reopening to restart them

@jmarolf jmarolf closed this Nov 17, 2016
@jmarolf jmarolf reopened this Nov 17, 2016
@jmarolf jmarolf merged commit 2e27639 into dotnet:dev15-rc2 Nov 17, 2016

10 checks passed

linux_debug_prtest Build finished.
Details
microbuild_prtest Build finished.
Details
perf_correctness_prtest Build finished.
Details
windows_build_correctness_prtest Build finished.
Details
windows_debug_unit32_prtest Build finished.
Details
windows_debug_unit64_prtest Build finished.
Details
windows_determinism_prtest Build finished.
Details
windows_eta_open_prtest Build finished.
Details
windows_release_unit32_prtest Build finished.
Details
windows_release_unit64_prtest Build finished.
Details
@Mpdreamz Mpdreamz referenced this pull request in editorconfig/editorconfig-visualstudio Nov 18, 2016
Open

Future of this project with regards to VS 2017 #49

@kuhlenh
Contributor
kuhlenh commented Nov 30, 2016

@jmarolf -- hey, so when making this work for Roslyn, I realized that it in our coding guidelines we call "predefined types" actually "language types". Perhaps let's reconsider the naming here? Might not be worth it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment