[ADD] dotnet solution migrate (draft) #44419

edvilme · 2024-10-23T20:52:56Z

Created dotnet solution migrate solutionfile.sln to generate a new solution file with the .slnx extension using https://github.com/microsoft/vs-solutionpersistence

Contributes to #40913 (dotnet sln migrate)

kasperk81 · 2024-10-24T02:00:33Z

unit tests upcoming?

Addresses #40913

I thought addressing that epic also includes:

dotnet new solution --format slnx (with alias -f slnx)
dotnet sln {add,remove,list} my.slnx my.csproj commands

edvilme · 2024-10-24T17:07:50Z

unit tests upcoming?

Hello. Yes, this is still WIP and will have a unit test that verifies if the command work. Additional testing on how the conversion is done, is more related to the https://github.com/microsoft/vs-solutionpersistence project as we just consume their API

kasperk81 · 2024-10-24T17:19:16Z

btw, current main and 9.0.200 branch can "build" .slnx #40913 (comment), so the test can assert on that to cover migration.

you may also consider changing Addresses #40913 to Contributes to #40913 so it doesn't close the issue on merge for additional work (dotnet new sln, dotnet sln {add,remove,list} my.slnx my.csproj), and keep this PR for dotnet sln migrate only.

Directory.Packages.props

nagilson

The change looks good so far :) I've left some thoughts below. Definitely add some tests like mentioned above. Also, please keep the PR in a draft state if it's still a WIP so it's clear to reviewers.

nagilson · 2024-10-24T22:20:17Z

src/Cli/dotnet/commands/dotnet-sln/LocalizableStrings.resx

@@ -177,4 +177,10 @@
  <data name="SolutionFolderHeader" xml:space="preserve">
    <value>Solution Folder(s)</value>
  </data>
-</root>
+  <data name="MigrateAppFullName" xml:space="preserve">
+    <value>Generate a .slnx solution file from a .sln solution file.</value>


Suggested change

<value>Generate a .slnx solution file from a .sln solution file.</value>

<value>Generate a .slnx file from a .sln file.</value>

I'm not sure whether this makes the wording clearer. @baronfel, thoughts?

nagilson · 2024-10-24T22:20:48Z

src/Cli/dotnet/commands/dotnet-sln/LocalizableStrings.resx

+    <value>Generate a .slnx solution file from a .sln solution file.</value>
+  </data>
+  <data name="SlnxGenerated" xml:space="preserve">
+    <value>Solution file {0} generated.</value>


Suggested change

<value>Solution file {0} generated.</value>

<value>.slnx file {0} generated.</value>

Same here, it would be good to get more feedback on this. I think it should indicate that a slnx file is being produced in particular.

nagilson · 2024-10-24T22:48:21Z

src/Cli/dotnet/commands/dotnet-sln/migrate/SlnMigrateCommandParser.cs

+
+        private static CliCommand ConstructCommand()
+        {
+            CliCommand command = new("migrate", LocalizableStrings.MigrateAppFullName);


@baronfel I read the spec and this looks good. But I wanted to check if there were any other options/flags you think this command should have?

I chatted with @baronfel and he told me that there were no current plans on adding more flags (at least for the time being), though I am happy to modify if needed

src/Cli/dotnet/commands/dotnet-sln/migrate/SlnMigrateCommand.cs

nagilson · 2024-10-24T23:27:10Z

src/Cli/dotnet/commands/dotnet-sln/migrate/SlnMigrateCommand.cs

+                throw new GracefulException(CommonLocalizableStrings.CouldNotFindSolutionIn, _slnFileFullPath);
+            }
+            string slnxFileFullPath = Path.ChangeExtension(_slnFileFullPath, "slnx");
+            Task task = ConvertToSlnxAsync(_slnFileFullPath, slnxFileFullPath, CancellationToken.None);


I believe you will want to call RunSynchronously() on this Task. But I haven't used Tasks in eons, so I would check with this.

nagilson · 2024-10-24T23:27:27Z

src/Cli/dotnet/commands/dotnet-sln/migrate/SlnMigrateCommand.cs

+            {
+                return;
+            }
+            SolutionModel solution = await serializer.OpenAsync(filePath, cancellationToken);


I think you should handle the possible SolutionError which this API can throw.

src/Cli/dotnet/commands/dotnet-sln/migrate/SlnMigrateCommand.cs

just-ero · 2024-10-25T03:13:30Z

Hi, I just want to make sure: this command won't require the solution path to be provided, right? It will take the solution file present in the current directory if no path is specified?

After migrating (I feel like convert would have been a nicer name), does the original solution file get deleted? Renamed? Does it stay?

nagilson

@just-ero raises a great point, many of our commands work if there is only one sln or project file in the directory where you don't need to specify a project or sln to take action on. Some of them even work if there are multiple and just picks one based on some order, like so:

string? potentialSln = Directory.GetFiles(arg, "*.sln", SearchOption.TopDirectoryOnly).FirstOrDefault();

I don't think this was in the original spec but we should support that. I am very familiar with the code to do that, so if you have questions let me know :)

src/Cli/dotnet/commands/dotnet-sln/migrate/SlnMigrateCommand.cs

nagilson · 2024-10-25T15:09:42Z

src/Cli/dotnet/commands/dotnet-sln/migrate/SlnMigrateCommand.cs

+            string slnxFileFullPath = Path.ChangeExtension(_slnFileFullPath, "slnx");
+            Task task = ConvertToSlnxAsync(_slnFileFullPath, slnxFileFullPath, CancellationToken.None);
+            _reporter.WriteLine(LocalizableStrings.SlnxGenerated, slnxFileFullPath);
+            return 0;


If we handle the SolutionError to print a better message, we should handle that and make sure we don't return 0.

nagilson · 2024-10-25T15:22:17Z

After migrating (I feel like convert would have been a nicer name), does the original solution file get deleted? Renamed? Does it stay?

@baronfel? I prefer 'migrate' personally and would keep the original solution file, deleting the file may be unexpected and cause pain there but it could be an option to provide.

edvilme · 2024-10-25T15:39:21Z

After migrating (I feel like convert would have been a nicer name), does the original solution file get deleted? Renamed? Does it stay?

@baronfel? I prefer 'migrate' personally and would keep the original solution file, deleting the file may be unexpected and cause pain there but it could be an option to provide.

I also prefer migrate and definitely think we should keep the original solution file at least until the rest of the dotnet sln commands are ready. Otherwise people would run the command with no way of undoing it and things could get messy (?)

I would advocate towards having a --replace flag and additional confirmation from the user if that was the path we wanted to follow

just-ero · 2024-10-25T15:44:47Z

string? potentialSln = Directory.GetFiles(arg, "*.sln", SearchOption.TopDirectoryOnly).FirstOrDefault();

Maybe this command should panic when more than one solution file is found?

Co-authored-by: kasperk81 <[email protected]>

baronfel · 2024-10-25T15:49:33Z

I've added details to the migration section on the linked issue. We should not delete the pre-existing .sln file, users are free to do that once they have tested and verified the slnx. solves their needs. We should use our existing solution-discovery mechanisms like we do for the other dotnet sln commands. We should error out if there are multiple .sln files in a directory that is used and no explicit sln argument is provided.

…into edvilme-migrate-slnx

nagilson · 2024-10-25T18:14:19Z

string? potentialSln = Directory.GetFiles(arg, "*.sln", SearchOption.TopDirectoryOnly).FirstOrDefault();
Maybe this command should panic when more than one solution file is found?

That is not how other commands behave, so I think its good to be consistent. This is the logic used by other commands.

edvilme · 2024-10-25T18:25:16Z

string? potentialSln = Directory.GetFiles(arg, "*.sln", SearchOption.TopDirectoryOnly).FirstOrDefault();
Maybe this command should panic when more than one solution file is found?
That is not how other commands behave, so I think its good to be consistent. This is the logic used by other commands.

What I've seen so far from SlnFileFactory is that it outputs an error message when two or more are found.

https://github.com/dotnet/sdk/blob/b0bf7b7b4dc36fc8d7b41dac631eced050473870/src/Cli/dotnet/SlnFileFactory.cs#L68C13-L73C14

[ADD] dotnet sln: Migrate

d5a0fe6

dotnet-issue-labeler bot added Area-CLI untriaged Request triage from a team member labels Oct 23, 2024

edvilme added 2 commits October 23, 2024 14:51

Add localized string

4569f85

Remove sdk.slnx

b70e48e

edvilme changed the title ~~[ADD] dotnet sln: Migrate~~ [ADD] dotnet solution: Migrate Oct 23, 2024

edvilme added 3 commits October 23, 2024 15:22

Add file path to file not found error

be17d2d

Update help message

6a42da4

Localizations

1530bd4

edvilme marked this pull request as ready for review October 23, 2024 23:09

edvilme requested a review from a team October 23, 2024 23:09

kasperk81 reviewed Oct 24, 2024

View reviewed changes

Directory.Packages.props Outdated Show resolved Hide resolved

kasperk81 reviewed Oct 24, 2024

View reviewed changes

Directory.Packages.props Show resolved Hide resolved

kasperk81 reviewed Oct 24, 2024

View reviewed changes

Directory.Packages.props Outdated Show resolved Hide resolved

nagilson reviewed Oct 24, 2024

View reviewed changes

edvilme marked this pull request as draft October 25, 2024 15:18

edvilme changed the title ~~[ADD] dotnet solution: Migrate~~ [ADD] dotnet solution migrate (draft) Oct 25, 2024

nagilson reviewed Oct 25, 2024

View reviewed changes

Apply suggestions from code review

35325c4

Co-authored-by: kasperk81 <[email protected]>

edvilme added 2 commits October 25, 2024 10:19

Infer sln file path if not specified

59eb737

Merge branch 'edvilme-migrate-slnx' of https://github.com/edvilme/sdk …

3ba0f84

…into edvilme-migrate-slnx

edvilme added 3 commits October 25, 2024 11:32

Updates checks for finding sln files

915058d

Move file path logic to different method

35235e5

Move file path logic to different method

6bee03d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADD] dotnet solution migrate (draft) #44419

[ADD] dotnet solution migrate (draft) #44419

edvilme commented Oct 23, 2024 •

edited

Loading

kasperk81 commented Oct 24, 2024 •

edited

Loading

edvilme commented Oct 24, 2024

kasperk81 commented Oct 24, 2024

nagilson left a comment •

edited

Loading

nagilson Oct 24, 2024

nagilson Oct 24, 2024

nagilson Oct 24, 2024

edvilme Oct 25, 2024

nagilson Oct 24, 2024

nagilson Oct 24, 2024

just-ero commented Oct 25, 2024

nagilson left a comment •

edited

Loading

nagilson Oct 25, 2024

nagilson commented Oct 25, 2024 •

edited

Loading

edvilme commented Oct 25, 2024 •

edited

Loading

just-ero commented Oct 25, 2024

baronfel commented Oct 25, 2024

nagilson commented Oct 25, 2024

edvilme commented Oct 25, 2024

	<value>Generate a .slnx solution file from a .sln solution file.</value>
	<value>Generate a .slnx file from a .sln file.</value>

	<value>Solution file {0} generated.</value>
	<value>.slnx file {0} generated.</value>

[ADD] dotnet solution migrate (draft) #44419

Are you sure you want to change the base?

[ADD] dotnet solution migrate (draft) #44419

Conversation

edvilme commented Oct 23, 2024 • edited Loading

kasperk81 commented Oct 24, 2024 • edited Loading

edvilme commented Oct 24, 2024

kasperk81 commented Oct 24, 2024

nagilson left a comment • edited Loading

Choose a reason for hiding this comment

nagilson Oct 24, 2024

Choose a reason for hiding this comment

nagilson Oct 24, 2024

Choose a reason for hiding this comment

nagilson Oct 24, 2024

Choose a reason for hiding this comment

edvilme Oct 25, 2024

Choose a reason for hiding this comment

nagilson Oct 24, 2024

Choose a reason for hiding this comment

nagilson Oct 24, 2024

Choose a reason for hiding this comment

just-ero commented Oct 25, 2024

nagilson left a comment • edited Loading

Choose a reason for hiding this comment

nagilson Oct 25, 2024

Choose a reason for hiding this comment

nagilson commented Oct 25, 2024 • edited Loading

edvilme commented Oct 25, 2024 • edited Loading

just-ero commented Oct 25, 2024

baronfel commented Oct 25, 2024

nagilson commented Oct 25, 2024

edvilme commented Oct 25, 2024

edvilme commented Oct 23, 2024 •

edited

Loading

kasperk81 commented Oct 24, 2024 •

edited

Loading

nagilson left a comment •

edited

Loading

nagilson left a comment •

edited

Loading

nagilson commented Oct 25, 2024 •

edited

Loading

edvilme commented Oct 25, 2024 •

edited

Loading