Add property to view .user file command line arguments #8494

ocallesp · 2022-09-15T19:05:36Z

Microsoft Reviewers: Open in CodeFlow

drewnoakes · 2022-09-16T03:50:35Z

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

+
+        XmlDocument doc = new XmlDocument();
+
+#pragma warning disable CA3075 // Insecure DTD processing in XML


Please verify this is safe to suppress. This may be a security concern.

Fixed. Setting XmlResolver to null as suggested in https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca3075#solution-6

Why is this #pragma still in the code? If you've fixed the issue, can you remove it?

For some reason the static analysis still believes this is unsafe.

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

drewnoakes · 2022-09-16T04:02:24Z

...Studio.ProjectSystem.Managed/ProjectSystem/Rules/PropertyPages/ProjectDebugPropertyPage.xaml

+  <StringProperty Name="StartArguments"
+                  DisplayName="ReadOnly .user file command line arguments"
+                  Description="Deprecated. This is only visible if there are command line arguments in the .user file to pass to the executable."
+                  ReadOnly="False">


Why not ReadOnly="True" here?

removed in last commit

I don't think this should be removed. I think it should be added.

Currently, this property will be editable. Yet I don't think that's what we want, is it? We want to display the value here, and allow the user to migrate the value from .user to .json.

Even if we set this ReadOnly="True", cps will mark it as read only because it thinks this is a common property.

...Studio.ProjectSystem.Managed/ProjectSystem/Rules/PropertyPages/ProjectDebugPropertyPage.xaml

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

docs/repo/property-pages/property-specification.md

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

docs/repo/property-pages/property-specification.md

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

drewnoakes · 2022-09-19T00:31:40Z

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

+
+    public Task HandleAsync(UnconfiguredProject project, IReadOnlyDictionary<string, string> editorMetadata)
+    {
+        _xmlFile = project.FullPath + UserSuffix;


Please check with the CPS team about whether it would be better to use CPS APIs to modify the .user file, rather than doing direct XML-to-disk changes here.

The cps apis marked this property as read only and doesn't allow to make changes to it.

The .user file is part of the loaded project; in general we shouldn't modify directly.

Do we understand why "StartArguments" is being treated as read-only?

tmeschter

We need to understand why the property is being treated as read-only.
Don't modify the .user file directly.
Please add unit tests.

tmeschter · 2022-09-19T22:05:11Z

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

+
+    public Task HandleAsync(UnconfiguredProject project, IReadOnlyDictionary<string, string> editorMetadata)
+    {
+        _xmlFile = project.FullPath + UserSuffix;


The .user file is part of the loaded project; in general we shouldn't modify directly.

Do we understand why "StartArguments" is being treated as read-only?

tmeschter · 2022-09-19T22:09:53Z

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

+
+    private async Task CopyUserArgsToLaunchProfileAsync(UnconfiguredProject project)
+    {
+        ILaunchSettingsProvider? launchSettingsProvider = project.Services.ExportProvider.GetExportedValue<ILaunchSettingsProvider>();


Any reason to do this rather than import the ILaunchSettingsProvider in a constructor?

Among other things it would make it easier to write unit tests for this.

I already tried importing ILaunchSettingsProvider in the constructor, but I got a lot of composition errors.

tmeschter · 2022-09-19T22:17:36Z

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

+    private const string UserSuffix = ".user";
+
+    private string? _xmlFile;
+    private List<string>? userFileCommandArgs;


_userFileCommandArgs

tmeschter · 2022-09-19T22:22:57Z

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

+
+        foreach (string? commandArg in userFileCommandArgs)
+        {
+            writableLaunchSettings.ActiveProfile.CommandLineArgs += " " + commandArg;


You need to account for:

CommandLineArgs may be null.

CommandLineArgs may be the empty string.

tmeschter · 2022-09-19T22:27:35Z

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs

+
+        foreach (string? commandArg in userFileCommandArgs)
+        {
+            writableLaunchSettings.ActiveProfile.CommandLineArgs += " " + commandArg;


Also, this is going to move the StartArguments to the command line arguments of the active profile. I see two potential problems:

The user may not be editing the active profile. In that case the arguments will just seem to disappear when they click the link.

StartArguments potentially affect launch/debug regardless of the launch profile, but this will move them to one specific launch profile.

I'm not sure either of those is really a problem, but we should think about those scenarios.

ocallesp · 2022-09-19T22:58:45Z

Converting this pr to draft since there is still unclear why StartArguments is treated as read-only in cps. Perhaps there is a bug in cps.

ocallesp force-pushed the user-file-command-line-arguments branch 5 times, most recently from c7f5b91 to d16a4f6 Compare September 15, 2022 20:42

ocallesp marked this pull request as ready for review September 15, 2022 20:42

ocallesp requested a review from a team as a code owner September 15, 2022 20:42

ocallesp changed the title ~~Add property to view user file command line arguments~~ Add property to view .user file command line arguments Sep 15, 2022

ocallesp force-pushed the user-file-command-line-arguments branch 3 times, most recently from 6c85bb7 to 83c06c9 Compare September 15, 2022 23:35

ocallesp requested a review from tmeschter September 16, 2022 00:52

drewnoakes reviewed Sep 16, 2022

View reviewed changes

ocallesp force-pushed the user-file-command-line-arguments branch 7 times, most recently from 9d43d61 to 03f5dad Compare September 18, 2022 14:09

drewnoakes reviewed Sep 19, 2022

View reviewed changes

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs Outdated Show resolved Hide resolved

drewnoakes reviewed Sep 19, 2022

View reviewed changes

docs/repo/property-pages/property-specification.md Outdated Show resolved Hide resolved

drewnoakes reviewed Sep 19, 2022

View reviewed changes

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs Outdated Show resolved Hide resolved

drewnoakes reviewed Sep 19, 2022

View reviewed changes

...operties/InterceptedProjectProperties/DebugPropertyPage/RemoveStartArgumentsActionHandler.cs Outdated Show resolved Hide resolved

drewnoakes reviewed Sep 19, 2022

View reviewed changes

ocallesp requested a review from drewnoakes September 19, 2022 16:41

ocallesp force-pushed the user-file-command-line-arguments branch from ece2787 to 9c68547 Compare September 19, 2022 17:58

tmeschter requested changes Sep 19, 2022

View reviewed changes

ocallesp marked this pull request as draft September 19, 2022 22:52

Add property to view command line arguments from the .user file

37acd35

ocallesp force-pushed the user-file-command-line-arguments branch from 9c68547 to 37acd35 Compare September 21, 2022 18:53

ocallesp closed this Sep 21, 2022

ocallesp deleted the user-file-command-line-arguments branch September 21, 2022 20:59

drewnoakes mentioned this pull request May 22, 2023

Warning/Notice when StartArguments interfere with launchSettings.json #8937

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add property to view .user file command line arguments #8494

Add property to view .user file command line arguments #8494

ocallesp commented Sep 15, 2022 •

edited

Loading

drewnoakes Sep 16, 2022

ocallesp Sep 18, 2022

drewnoakes Sep 19, 2022

ocallesp Sep 19, 2022

drewnoakes Sep 16, 2022

ocallesp Sep 16, 2022

drewnoakes Sep 19, 2022

ocallesp Sep 19, 2022

drewnoakes Sep 19, 2022

ocallesp Sep 19, 2022

tmeschter Sep 19, 2022

tmeschter left a comment

tmeschter Sep 19, 2022

tmeschter Sep 19, 2022

tmeschter Sep 19, 2022

ocallesp Sep 19, 2022

tmeschter Sep 19, 2022

tmeschter Sep 19, 2022

tmeschter Sep 19, 2022

ocallesp commented Sep 19, 2022


		XmlDocument doc = new XmlDocument();

		#pragma warning disable CA3075 // Insecure DTD processing in XML

Add property to view .user file command line arguments #8494

Add property to view .user file command line arguments #8494

Conversation

ocallesp commented Sep 15, 2022 • edited Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeschter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocallesp commented Sep 19, 2022

ocallesp commented Sep 15, 2022 •

edited

Loading