You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
built all five imported projects with analyzers enabled
verified package, menu, and wizard codebase declarations against Roslyn's GeneratePkgDef.targets contract
verified representative git log --follow history through the source renames and Roslyn relocation
verified the final diff with git diff --check
performed independent pre-PR reviews and addressed the identified VSIX, pkgdef, and dependency issues
Ide.slnf validation was attempted, but the complete filter requires Windows/.NET Framework targeting packs unavailable on macOS; CI will provide the Windows packaging and full IDE validation
Test Insertion shows EditorConfigTemplates are properly inserted with no regressions.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The imported wizard/command code has at least one user-visible correctness issue (possible null content passed to File.WriteAllText) plus a likely background-thread type-initialization failure risk in VSHelpers that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite Findings: 3 · 2
New issues introduced by this change (5)
Severity
Finding
src/VisualStudio/EditorConfig/Wizard/Generator/EditorConfigFileGenerator.cs — GetEditorconfigFileContents(...) can return null (e.g., Roslyn generator service…
src/VisualStudio/EditorConfig/Wizard/Generator/RoslynEditorConfigFileGenerator.cs — ServiceProvider.GlobalProvider.GetService(...) is cast with a direct cast to IComponentModel,…
src/VisualStudio/EditorConfig/Wizard/Utilities/VSHelpers.cs — The static DTE property is initialized by calling GetService(...), which enforces UI-thread…
src/VisualStudio/EditorConfig/Wizard/Logging/TelemetrySessionAggregator.cs — Typo in the DEBUG-only helper parameter name descrption -> description. Keeping these debug…
src/VisualStudio/EditorConfig/Wizard/WizardResource.resx — User-facing English resource string has a grammatical error: “already exist” should be “already…
What changed in this PR
This PR imports the Visual Studio EditorConfig item template + wizard + context-menu command + setup/packaging projects into Roslyn under src/VisualStudio/EditorConfig/, wiring them into Roslyn’s solution structure and centralized package/version management.
Changes:
Adds new EditorConfig template, wizard, command, and setup projects (VSIX/SWR/pkgdef resources and localization assets).
Integrates the new projects into Roslyn.slnx and Ide.slnf.
Extends central package versions and repo docs to reflect the new VisualStudio/EditorConfig subtree.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
There are confirmed robustness and privacy/telemetry issues in the newly imported Wizard code paths that should be addressed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite Findings: 1 · 3 · 2
New issues introduced by this change (1)
Severity
Finding
src/VisualStudio/EditorConfig/Wizard/Logging/Messages/TemplateInfo.cs — TemplateInfo currently logs raw replacementsDictionary values to telemetry. The VS template…
Pre-existing issues (5)
Severity
Finding
src/VisualStudio/EditorConfig/Wizard/Utilities/VSHelpers.cs — The static DTE property is initialized by calling GetService(...), which enforces UI-thread… View comment
src/VisualStudio/EditorConfig/Wizard/Generator/RoslynEditorConfigFileGenerator.cs — ServiceProvider.GlobalProvider.GetService(...) is cast with a direct cast to IComponentModel,… View comment
src/VisualStudio/EditorConfig/Wizard/WizardResource.resx — User-facing English resource string has a grammatical error: “already exist” should be “already… View comment
src/VisualStudio/EditorConfig/Wizard/Logging/TelemetrySessionAggregator.cs — Typo in the DEBUG-only helper parameter name descrption -> description. Keeping these debug… View comment
Directory.EnumerateFiles(..., SearchOption.AllDirectories) can throw (e.g., UnauthorizedAccessException/IOException) when walking a folder tree, especially for the AnyCode folder scenario. Since this is used to decide whether creation is supported, it’s better to treat enumeration failures as “no matching files” (or otherwise handle errors) rather than crashing the command.
VSHelpers.DTE is initialized via a type initializer that calls ThreadHelper.ThrowIfNotOnUIThread(). This can throw if the type is first touched off the UI thread (and also caches a potentially-null service). Consider making DTE a computed property and have GetService throw a clear exception when the service is unavailable, rather than returning null and failing later. src/VisualStudio/EditorConfig/Wizard/Generator/EditorConfigFileGenerator.cs:113
GetEditorconfigFileContents(...) can return null (e.g., if IEditorConfigGenerator isn’t available or throws), but WriteFile passes it directly to File.WriteAllText, which will throw ArgumentNullException. Consider falling back to the built-in DotNet/default templates (or failing gracefully) when generation returns null. src/VisualStudio/EditorConfig/Wizard/Generator/RoslynEditorConfigFileGenerator.cs:24
ServiceProvider.GlobalProvider.GetService(typeof(SComponentModel)) can return null; the direct cast to IComponentModel would throw. Also the error message has a small grammar issue (missing "to"). Consider using as IComponentModel and keeping the failure path non-throwing. src/VisualStudio/EditorConfig/Wizard/Logging/TelemetrySessionAggregator.cs:92
Typo in parameter name descrption (and propagated to the output string). This is DEBUG-only, but it’s still easy to fix for readability/maintainability. src/VisualStudio/EditorConfig/Wizard/WizardResource.resx:122
The user-facing English resource string is grammatically incorrect ("already exist" → "already exists"). Since the corresponding .xlf files carry the source text too, this likely needs to be updated via the normal RESX→XLF flow so they stay in sync.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Several correctness and quality issues (telemetry property loss, resource-string grammar/localization sync, and template/IO robustness bugs) should be addressed before merging.
Review tier: Lite Findings: 1 · 3 · 2
Pre-existing issues (6)
Severity
Finding
src/VisualStudio/EditorConfig/Wizard/Logging/Messages/TemplateInfo.cs — TemplateInfo currently logs raw replacementsDictionary values to telemetry. The VS template… View comment
src/VisualStudio/EditorConfig/Wizard/Utilities/VSHelpers.cs — The static DTE property is initialized by calling GetService(...), which enforces UI-thread… View comment
src/VisualStudio/EditorConfig/Wizard/Generator/RoslynEditorConfigFileGenerator.cs — ServiceProvider.GlobalProvider.GetService(...) is cast with a direct cast to IComponentModel,… View comment
src/VisualStudio/EditorConfig/Wizard/WizardResource.resx — User-facing English resource string has a grammatical error: “already exist” should be “already… View comment
src/VisualStudio/EditorConfig/Wizard/Logging/TelemetrySessionAggregator.cs — Typo in the DEBUG-only helper parameter name descrption -> description. Keeping these debug… View comment
The naming-symbol spec for type parameters appears to be incorrect: dotnet_naming_symbols.type_parameters.applicable_kinds = namespace. This prevents the type_parameters_should_be_tpascalcase rule from ever matching type parameters. (This line appears twice in the template constants.)
This issue also appears on line 669 of the same file. src/VisualStudio/EditorConfig/Wizard/Utilities/VSHelpers.cs:63
Directory.EnumerateFiles(..., SearchOption.AllDirectories) can throw (e.g., UnauthorizedAccessException/IOException) when walking large trees, which would crash the command path that uses IsDotnet(directory). Since this is just a heuristic, it should fail closed and return false on IO/access errors.
The English resource string has a grammar error: "already exist" should be "already exists". This string is referenced by the generated Designer file and all XLF entries, so those will need to be regenerated/updated to keep localization in sync. src/VisualStudio/EditorConfig/Wizard/Logging/Messages/TemplateInfo.cs:30
TemplateInfo emits multiple MessageData entries with the same Name ("ReplacementsDictionaryValue"). LoggerHelpers.SetProperties deduplicates by property name, so only the first replacement value will be recorded and the rest are silently dropped. src/VisualStudio/EditorConfig/Wizard/Logging/TelemetrySessionAggregator.cs:95
In the DEBUG-only telemetry output helper, the parameter name "descrption" is misspelled (and used in the interpolated output). This makes the debug tracing harder to read/search. src/VisualStudio/EditorConfig/Wizard/Generator/RoslynEditorConfigFileGenerator.cs:31
The logged fault description has a typo/grammar issue ("Unable call"), which makes telemetry/error logs less searchable. src/VisualStudio/EditorConfig/Wizard/Utilities/TemplateConstants.cs:670
Same issue as earlier in this file: the type-parameter symbol spec uses applicable_kinds = namespace, which means the type-parameter naming rule can't apply.
Keep the insertion component name while preserving the package ID consumed by existing Visual Studio template manifests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cadde354-c3ae-424c-b0c3-11f3e3662848
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
There are merge-blocking correctness/privacy issues in the newly added wizard/telemetry code paths (null file content write risk and logging of potentially sensitive template replacement values).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite Findings: 2 · 3 · 2
New issues introduced by this change (1)
Severity
Finding
src/VisualStudio/EditorConfig/Wizard/Logging/Messages/TemplateInfo.cs — TemplateInfo currently logs every replacementsDictionary value into telemetry. Template-wizard…
Pre-existing issues (6)
Severity
Finding
src/VisualStudio/EditorConfig/Wizard/Logging/Messages/TemplateInfo.cs — TemplateInfo currently logs raw replacementsDictionary values to telemetry. The VS template… View comment
src/VisualStudio/EditorConfig/Wizard/Utilities/VSHelpers.cs — The static DTE property is initialized by calling GetService(...), which enforces UI-thread… View comment
src/VisualStudio/EditorConfig/Wizard/Generator/RoslynEditorConfigFileGenerator.cs — ServiceProvider.GlobalProvider.GetService(...) is cast with a direct cast to IComponentModel,… View comment
src/VisualStudio/EditorConfig/Wizard/WizardResource.resx — User-facing English resource string has a grammatical error: “already exist” should be “already… View comment
src/VisualStudio/EditorConfig/Wizard/Logging/TelemetrySessionAggregator.cs — Typo in the DEBUG-only helper parameter name descrption -> description. Keeping these debug… View comment
LogCreateOperation currently reports Roslyn generation operations for any C#/VB language, even when isDotnet is false (so we actually write DefaultFileContent*). This makes telemetry/diagnostics misleading; the switch should prioritize isDotnet/isAtSolutionLevel first, then language only for the Roslyn-generator cases. src/VisualStudio/EditorConfig/Wizard/Generator/EditorConfigFileGenerator.cs:115
GetEditorconfigFileContents can return null (e.g., RoslynEditorConfigFileGenerator.Generate returns null or the language falls through to _ => null), which will throw ArgumentNullException in File.WriteAllText. Please ensure we always write a non-null fallback (and optionally log/telemetry when generation fails). src/VisualStudio/EditorConfig/Command/VSCommandTable.cs:14
PackageGuids.AddEditorConfig is a mutable static field; it should be readonly like AddEditorConfigCmdSet to avoid accidental reassignment and to match common Roslyn patterns for GUID constants. src/VisualStudio/EditorConfig/Wizard/Logging/TelemetrySessionAggregator.cs:95
Typo in parameter name descrption makes this debug-only helper harder to read/search; rename to description and update the interpolated string accordingly.
Grammar: "already exist" should be "already exists" in the English resource string. If you update the English source, please also update the corresponding text in the WizardResource.*.xlf files to keep localization sources in sync.
The reason will be displayed to describe this comment to others. Learn more.
Signing off in that this code is moving, but I imagine there's some useful follow-ups to reduce the number of setup packages, or even just DLLs entirely.
- **EditorConfig templates** (`src/VisualStudio/EditorConfig/`): item templates, generation wizard, context-menu command, VSIX projects, and Visual Studio insertion setup
- The setup insertion component is `Templates.Editorconfig.Setup`, but its SWR package identity must remain `Templates.Editorconfig.SolutionFile.Setup` because existing Visual Studio template packages depend on that ID.
The reason will be displayed to describe this comment to others. Learn more.
I'd imagine we can just put this into the existing VSIX instead? Sure we'd have to remove a few lines of setup authoring when we insert but is that easier? (I admit I don't know the last time we removed a component, so maybe there's a surprise there)
- **EditorConfig templates** (`src/VisualStudio/EditorConfig/`): item templates, generation wizard, context-menu command, VSIX projects, and Visual Studio insertion setup
- The setup insertion component is `Templates.Editorconfig.Setup`, but its SWR package identity must remain `Templates.Editorconfig.SolutionFile.Setup` because existing Visual Studio template packages depend on that ID.
The reason will be displayed to describe this comment to others. Learn more.
Which command is this defined for? Are we loading this package/DLL just to proffer a command like this? I'm just wondering if we were to pull this into any of our existing assemblies we can remove a DLL load.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
jasonmalinowski
left a comment
Summary
dotnet/templatessrc/VisualStudio/EditorConfigSource repository: https://github.com/dotnet/templates
Source ref:
mainat454eb94c94bb02887843101928ad2f172b9f2acaValidation
GeneratePkgDef.targetscontractgit log --followhistory through the source renames and Roslyn relocationgit diff --checkIde.slnfvalidation was attempted, but the complete filter requires Windows/.NET Framework targeting packs unavailable on macOS; CI will provide the Windows packaging and full IDE validationTest Insertion shows EditorConfigTemplates are properly inserted with no regressions.
Microsoft Reviewers: Open in CodeFlow