-
Notifications
You must be signed in to change notification settings - Fork 10
Fix csharp_style_namespace_declarations EditorConfig support for step definition generation #106
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
base: main
Are you sure you want to change the base?
Changes from 15 commits
dc21623
f5b0c53
61bebbc
1004f48
ee82784
ad7fb00
655c69b
31235ce
3ac3a40
8d61bc8
d4010d2
b84db03
3b6606e
9d0ae71
ce41e66
36f5be8
2c49097
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| namespace Reqnroll.VisualStudio.Configuration; | ||
|
|
||
| public class CSharpCodeGenerationConfiguration | ||
| { | ||
| /// <summary> | ||
| /// Specifies the namespace declaration style for generated C# code. | ||
| /// Uses file-scoped namespaces when set to "file_scoped", otherwise uses block-scoped namespaces. | ||
| /// </summary> | ||
| [EditorConfigSetting("csharp_style_namespace_declarations")] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it only works with and not with Why is that? Please fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed the root cause! Added GetEditorConfigOptionsByPath method to get EditorConfig settings for the target .cs file path instead of the source .feature file. Now [*.cs] sections in .editorconfig will work correctly. |
||
| public string NamespaceDeclarationStyle { get; set; } = "block_scoped"; | ||
|
|
||
| /// <summary> | ||
| /// Determines if file-scoped namespaces should be used based on the EditorConfig setting. | ||
| /// </summary> | ||
| public bool UseFileScopedNamespaces => | ||
| NamespaceDeclarationStyle != null && | ||
| NamespaceDeclarationStyle.StartsWith("file_scoped", StringComparison.OrdinalIgnoreCase); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,23 @@ | ||
| #nullable disable | ||
|
|
||
| using System.Text; | ||
|
|
||
| namespace Reqnroll.VisualStudio.Editor.Commands; | ||
|
|
||
| [Export(typeof(IDeveroomFeatureEditorCommand))] | ||
| public class DefineStepsCommand : DeveroomEditorCommandBase, IDeveroomFeatureEditorCommand | ||
| { | ||
| private readonly IEditorConfigOptionsProvider _editorConfigOptionsProvider; | ||
|
|
||
| [ImportingConstructor] | ||
| public DefineStepsCommand( | ||
| IIdeScope ideScope, | ||
| IBufferTagAggregatorFactoryService aggregatorFactory, | ||
| IDeveroomTaggerProvider taggerProvider) | ||
| IDeveroomTaggerProvider taggerProvider, | ||
| IEditorConfigOptionsProvider editorConfigOptionsProvider) | ||
| : base(ideScope, aggregatorFactory, taggerProvider) | ||
| { | ||
| _editorConfigOptionsProvider = editorConfigOptionsProvider; | ||
| } | ||
|
|
||
| public override DeveroomEditorCommandTargetKey[] Targets => new[] | ||
|
|
@@ -101,7 +108,7 @@ public override bool PreExec(IWpfTextView textView, DeveroomEditorCommandTargetK | |
| switch (viewModel.Result) | ||
| { | ||
| case CreateStepDefinitionsDialogResult.Create: | ||
| SaveAsStepDefinitionClass(projectScope, combinedSnippet, viewModel.ClassName, indent, newLine); | ||
| SaveAsStepDefinitionClass(projectScope, combinedSnippet, viewModel.ClassName, indent, newLine, textView); | ||
| break; | ||
| case CreateStepDefinitionsDialogResult.CopyToClipboard: | ||
| Logger.LogVerbose($"Copy to clipboard: {combinedSnippet}"); | ||
|
|
@@ -114,7 +121,7 @@ public override bool PreExec(IWpfTextView textView, DeveroomEditorCommandTargetK | |
| } | ||
|
|
||
| private void SaveAsStepDefinitionClass(IProjectScope projectScope, string combinedSnippet, string className, | ||
| string indent, string newLine) | ||
| string indent, string newLine, IWpfTextView textView) | ||
| { | ||
| string targetFolder = projectScope.ProjectFolder; | ||
| var projectSettings = projectScope.GetProjectSettings(); | ||
|
|
@@ -130,29 +137,56 @@ private void SaveAsStepDefinitionClass(IProjectScope projectScope, string combin | |
| var isSpecFlow = projectTraits.HasFlag(ReqnrollProjectTraits.LegacySpecFlow) || projectTraits.HasFlag(ReqnrollProjectTraits.SpecFlowCompatibility); | ||
| var libraryNameSpace = isSpecFlow ? "SpecFlow" : "Reqnroll"; | ||
|
|
||
| var template = "using System;" + newLine + | ||
| $"using {libraryNameSpace};" + newLine + | ||
| newLine + | ||
| $"namespace {fileNamespace}" + newLine + | ||
| "{" + newLine + | ||
| $"{indent}[Binding]" + newLine + | ||
| $"{indent}public class {className}" + newLine + | ||
| $"{indent}{{" + newLine + | ||
| combinedSnippet + | ||
| $"{indent}}}" + newLine + | ||
| "}" + newLine; | ||
| // Get C# code generation configuration from EditorConfig | ||
| var csharpConfig = new CSharpCodeGenerationConfiguration(); | ||
| var editorConfigOptions = _editorConfigOptionsProvider.GetEditorConfigOptions(textView); | ||
| editorConfigOptions.UpdateFromEditorConfig(csharpConfig); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found and fixed the root cause! There was a critical bug in Fixed in commit 9d0ae71:
The fix ensures that |
||
|
|
||
| // Build template with common structure | ||
| var template = new StringBuilder(); | ||
|
||
| template.AppendLine("using System;"); | ||
| template.AppendLine($"using {libraryNameSpace};"); | ||
| template.AppendLine(); | ||
|
|
||
| // Determine indentation level based on namespace style | ||
| var classIndent = csharpConfig.UseFileScopedNamespaces ? "" : indent; | ||
|
|
||
| // Add namespace declaration | ||
| if (csharpConfig.UseFileScopedNamespaces) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot deduplicate this part, it's almost the same as the else There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deduplicated the template generation code by extracting the common parts (using statements) and only varying the namespace-specific sections. This eliminates the code duplication while maintaining the exact same functionality. Fixed in commit 655c69b. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot this is still duplicated? 90% of the difference is the indent, Maybe convert it to a stringbuilder? So I mean the template += in the if else below There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed the code duplication by refactoring to use StringBuilder approach. The template generation now builds the namespace and class parts separately with appropriate indentation, eliminating 90% of the redundant code between the if-else branches. Fixed in commit d4e086a. |
||
| { | ||
| template.AppendLine($"namespace {fileNamespace};"); | ||
| template.AppendLine(); | ||
| } | ||
| else | ||
| { | ||
| template.AppendLine($"namespace {fileNamespace}"); | ||
| template.AppendLine("{"); | ||
| } | ||
|
|
||
| // Add class declaration (common structure with appropriate indentation) | ||
| template.AppendLine($"{classIndent}[Binding]"); | ||
| template.AppendLine($"{classIndent}public class {className}"); | ||
| template.AppendLine($"{classIndent}{{"); | ||
| template.Append(combinedSnippet); | ||
|
||
| template.AppendLine($"{classIndent}}}"); | ||
|
|
||
| // Close namespace if block-scoped | ||
| if (!csharpConfig.UseFileScopedNamespaces) | ||
| { | ||
| template.AppendLine("}"); | ||
| } | ||
|
|
||
| var targetFile = FileDetails | ||
| .FromPath(targetFolder, className + ".cs") | ||
| .WithCSharpContent(template); | ||
| .WithCSharpContent(template.ToString()); | ||
|
|
||
| if (IdeScope.FileSystem.File.Exists(targetFile.FullName)) | ||
| if (IdeScope.Actions.ShowSyncQuestion("Overwrite file?", | ||
| $"The selected step definition file '{targetFile}' already exists. By overwriting the existing file you might lose work. {Environment.NewLine}Do you want to overwrite the file?", | ||
| defaultButton: MessageBoxResult.No) != MessageBoxResult.Yes) | ||
| return; | ||
|
|
||
| projectScope.AddFile(targetFile, template); | ||
| projectScope.AddFile(targetFile, template.ToString()); | ||
| projectScope.IdeScope.Actions.NavigateTo(new SourceLocation(targetFile, 9, 1)); | ||
| IDiscoveryService discoveryService = projectScope.GetDiscoveryService(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| using Reqnroll.VisualStudio.Configuration; | ||
| using Reqnroll.VisualStudio.Editor.Services.EditorConfig; | ||
| using Xunit; | ||
|
|
||
| namespace Reqnroll.VisualStudio.Tests.Configuration; | ||
|
|
||
| public class CSharpCodeGenerationConfigurationTests | ||
| { | ||
| [Fact] | ||
| public void UseFileScopedNamespaces_WhenFileScopedSet_ReturnsTrue() | ||
| { | ||
| // Arrange | ||
| var config = new CSharpCodeGenerationConfiguration | ||
| { | ||
| NamespaceDeclarationStyle = "file_scoped" | ||
| }; | ||
|
|
||
| // Act & Assert | ||
| Assert.True(config.UseFileScopedNamespaces); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UseFileScopedNamespaces_WhenFileScopedWithSeveritySet_ReturnsTrue() | ||
| { | ||
| // Arrange | ||
| var config = new CSharpCodeGenerationConfiguration | ||
| { | ||
| NamespaceDeclarationStyle = "file_scoped:warning" | ||
| }; | ||
|
|
||
| // Act & Assert | ||
| Assert.True(config.UseFileScopedNamespaces); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UseFileScopedNamespaces_WhenBlockScopedSet_ReturnsFalse() | ||
| { | ||
| // Arrange | ||
| var config = new CSharpCodeGenerationConfiguration | ||
| { | ||
| NamespaceDeclarationStyle = "block_scoped" | ||
| }; | ||
|
|
||
| // Act & Assert | ||
| Assert.False(config.UseFileScopedNamespaces); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UseFileScopedNamespaces_WhenDefaultValue_ReturnsFalse() | ||
| { | ||
| // Arrange | ||
| var config = new CSharpCodeGenerationConfiguration(); | ||
|
|
||
| // Act & Assert | ||
| Assert.False(config.UseFileScopedNamespaces); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UseFileScopedNamespaces_WhenNullValue_ReturnsFalse() | ||
| { | ||
| // Arrange | ||
| var config = new CSharpCodeGenerationConfiguration | ||
| { | ||
| NamespaceDeclarationStyle = null | ||
| }; | ||
|
|
||
| // Act & Assert | ||
| Assert.False(config.UseFileScopedNamespaces); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UseFileScopedNamespaces_WhenUnknownValue_ReturnsFalse() | ||
| { | ||
| // Arrange | ||
| var config = new CSharpCodeGenerationConfiguration | ||
| { | ||
| NamespaceDeclarationStyle = "unknown_style" | ||
| }; | ||
|
|
||
| // Act & Assert | ||
| Assert.False(config.UseFileScopedNamespaces); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UpdateFromEditorConfig_WhenFileScopedValue_SetsCorrectValue() | ||
| { | ||
| // Arrange | ||
| var config = new CSharpCodeGenerationConfiguration(); | ||
| var editorConfigOptions = new TestEditorConfigOptions("file_scoped:silent"); | ||
|
|
||
| // Act | ||
| editorConfigOptions.UpdateFromEditorConfig(config); | ||
|
|
||
| // Assert | ||
| Assert.Equal("file_scoped:silent", config.NamespaceDeclarationStyle); | ||
| Assert.True(config.UseFileScopedNamespaces); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UpdateFromEditorConfig_WhenBlockScopedValue_SetsCorrectValue() | ||
| { | ||
| // Arrange | ||
| var config = new CSharpCodeGenerationConfiguration(); | ||
| var editorConfigOptions = new TestEditorConfigOptions("block_scoped"); | ||
|
|
||
| // Act | ||
| editorConfigOptions.UpdateFromEditorConfig(config); | ||
|
|
||
| // Assert | ||
| Assert.Equal("block_scoped", config.NamespaceDeclarationStyle); | ||
| Assert.False(config.UseFileScopedNamespaces); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UpdateFromEditorConfig_WhenNoValue_KeepsDefault() | ||
| { | ||
| // Arrange | ||
| var config = new CSharpCodeGenerationConfiguration(); | ||
| var editorConfigOptions = new TestEditorConfigOptions(null); | ||
|
|
||
| // Act | ||
| editorConfigOptions.UpdateFromEditorConfig(config); | ||
|
|
||
| // Assert | ||
| Assert.Equal("block_scoped", config.NamespaceDeclarationStyle); // Should keep default | ||
| Assert.False(config.UseFileScopedNamespaces); | ||
| } | ||
| } | ||
|
|
||
| // Test EditorConfig options provider that simulates reading specific values | ||
| public class TestEditorConfigOptions : IEditorConfigOptions | ||
| { | ||
| private readonly string _namespaceStyle; | ||
|
|
||
| public TestEditorConfigOptions(string namespaceStyle) | ||
| { | ||
| _namespaceStyle = namespaceStyle; | ||
| } | ||
|
|
||
| public TResult GetOption<TResult>(string editorConfigKey, TResult defaultValue) | ||
| { | ||
| if (editorConfigKey == "csharp_style_namespace_declarations" && _namespaceStyle != null) | ||
| { | ||
| return (TResult)(object)_namespaceStyle; | ||
| } | ||
|
|
||
| return defaultValue; | ||
| } | ||
|
|
||
| public bool GetBoolOption(string editorConfigKey, bool defaultValue) | ||
| { | ||
| return defaultValue; | ||
| } | ||
| } |
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.
@copilot why this change?
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.
You're right - that change shouldn't be in the PR. I added it to test the functionality but it modifies the project's own EditorConfig. Reverted in commit coming up.