-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[ADD] dotnet solution migrate (draft) #44419
base: main
Are you sure you want to change the base?
Conversation
unit tests upcoming?
I thought addressing that epic also includes:
|
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 |
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 |
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.
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.
@@ -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> |
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.
<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?
<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> |
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.
<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.
|
||
private static CliCommand ConstructCommand() | ||
{ | ||
CliCommand command = new("migrate", LocalizableStrings.MigrateAppFullName); |
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.
@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?
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.
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
Outdated
Show resolved
Hide resolved
throw new GracefulException(CommonLocalizableStrings.CouldNotFindSolutionIn, _slnFileFullPath); | ||
} | ||
string slnxFileFullPath = Path.ChangeExtension(_slnFileFullPath, "slnx"); | ||
Task task = ConvertToSlnxAsync(_slnFileFullPath, slnxFileFullPath, CancellationToken.None); |
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.
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.
{ | ||
return; | ||
} | ||
SolutionModel solution = await serializer.OpenAsync(filePath, cancellationToken); |
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.
I think you should handle the possible SolutionError which this API can throw.
src/Cli/dotnet/commands/dotnet-sln/migrate/SlnMigrateCommand.cs
Outdated
Show resolved
Hide resolved
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 |
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.
@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 :)
string slnxFileFullPath = Path.ChangeExtension(_slnFileFullPath, "slnx"); | ||
Task task = ConvertToSlnxAsync(_slnFileFullPath, slnxFileFullPath, CancellationToken.None); | ||
_reporter.WriteLine(LocalizableStrings.SlnxGenerated, slnxFileFullPath); | ||
return 0; |
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.
If we handle the SolutionError to print a better message, we should handle that and make sure we don't return 0.
@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 I would advocate towards having a |
Maybe this command should panic when more than one solution file is found? |
Co-authored-by: kasperk81 <[email protected]>
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 |
…into edvilme-migrate-slnx
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 |
Created
dotnet solution migrate solutionfile.sln
to generate a new solution file with the .slnx extension using https://github.com/microsoft/vs-solutionpersistenceContributes to #40913 (
dotnet sln migrate
)