-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add command 'dotnet project convert' #47500
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
Conversation
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.
Pull Request Overview
This pull request adds a new "dotnet project add" command that creates a project folder and generates a non-virtual project file based on an entry point file. It introduces:
- A new command implementation (ProjectAddCommand) and parser (ProjectAddCommandParser) to handle the add operation.
- Updates to the virtual project building logic and project command integration.
- Minor type qualification updates for consistency in related modules.
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/Cli/dotnet/commands/dotnet-project/add/ProjectAddCommand.cs | Implements the new add command; creates a project folder and moves/updates files accordingly. |
src/Cli/dotnet/commands/dotnet-project/add/ProjectAddCommandParser.cs | Adds the parser for the new command. |
src/Cli/dotnet/commands/dotnet-project/ProjectCommandParser.cs | Integrates the new subcommand into the project command. |
src/Cli/dotnet/commands/VirtualProjectBuildingCommand.cs | Adds common project properties and updates project file generation. |
src/Cli/dotnet/Parser.cs | Registers the new project command. |
src/Cli/dotnet/Extensions/ProjectExtensions.cs | Updates parameter types for consistency. |
src/Cli/dotnet/MsbuildProject.cs | Updates type qualification for evaluated projects. |
Comments suppressed due to low confidence (1)
src/Cli/dotnet/commands/dotnet-project/add/ProjectAddCommand.cs:23
- [nitpick] The variable name 'file' is ambiguous and may benefit from a more descriptive name such as 'fullFilePath' to clarify its purpose.
string file = Path.GetFullPath(_file);
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.
LGTM.
I did have a question about the verbiage of dotnet project add
given that we have dotnet add <projectfile>
as an existing command, but thats for the spec, and this PR matches what's currently in the spec.
You mean |
Correct - we prefer noun-first as much as possible starting in 10.0. |
We'll probably talk about this in design but I really don't like the |
It feels like a verb like convert/migrate/move would be appropriate here. But not suggesting to rename the command in this PR. |
src/Cli/dotnet/commands/dotnet-project/add/ProjectAddCommand.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Rikki Gibson <[email protected]>
@MiYanni for a review, thanks |
I'd like to come up with a good answer for this before taking a real look at this. Project feels wrong to me. I don't have a good answer to replace it yet; my best is library. |
Ideas for other command names, with assumption that each accepts file name as an argument:
What ChatGPT settled on:
https://chatgpt.com/share/67d4cad7-c7a4-8012-85f2-a0eb5286553e |
I have a PR for |
I like 'dotnet project convert' quite a lot (lots of space for like a --from-file, or --from-Xyz if we need it). I'm also looking at a far future with a 'dotnet project upgrade' sibling command, and that feels pretty nice. |
Okay, I like that as well, and I can change to that easily. Thanks. |
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 So, is it intended that every CLI surface change requires these 3 completion tests to have changes? It looks like it requires at least some Bash, PowerShell, and ZSH knowledge to do this, which is a pretty high bar.
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.
yes, it's intended. the CLI interface is a public interface, and we need to be careful about it the same way as we would a REST response or any other API.
these tests are one kind of gate in that regard.
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.
It looks like it requires at least some Bash, PowerShell, and ZSH knowledge to do this, which is a pretty high bar.
Note that these are automatically generated snapshots.
@@ -300,7 +297,7 @@ public void ClassLibrary_EntryPointFileExists() | |||
.WithWorkingDirectory(testInstance.Path) | |||
.Execute() | |||
.Should().Fail() | |||
.And.HaveStdErrContaining(s_noTopLevelStatements); | |||
.And.HaveStdOutContaining("error CS5001:"); // Program does not contain a static 'Main' method suitable for an entry point |
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.
Is the word "error" localized? Meaning, would this word change if it was run in a different language environment?
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.
Yes, it should be. Note that this is the output of the Roslyn compiler. Not sure why you are asking? It doesn't look like sdk tests run in different locales and there are many tests that verify localizable strings against their English variant. But for example, in Roslyn tests, we don't do that as we run the tests also on non-English locales intentionally. If you intend to do that as well in sdk in the future, let me know, I will try to rewrite my tests so they are locale-insensitive.
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.
Some people run SDK tests locally. They would get localized outputs for this sort of thing, so this test wouldn't work for them as-is.
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.
Makes sense, I can follow up with a PR that ensures RunFileTests and DotnetProjectConvertTests can run on localized machines.
But I want to reiterate that this is a pre-existing problem for many other tests, e.g., https://github.com/search?q=repo%3Adotnet%2Fsdk%20HaveStdErrContaining&type=code
I have also removed the check for top-level statements because I've realized that
<DefineConstants>
in an implicit build file or on the command line, they would not be considered when using Roslyn API to detect top-level statements but they would be recognized during the build itself, potentially leading to inconsistencies (I've added tests for that),dotnet run util.cs
, we can do that check in Roslyn instead, avoiding the<DefineConstants>
problem and reusing its logic for detecting entrypoints, hence also recognizing all forms of Main methods, not just top-level statements.