-
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
dotnet CLI: Update commands folder names #47746
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 PR reorganizes the dotnet CLI commands by moving them into dedicated folders named after each command, setting the stage for further refactoring such as namespace updates.
- Introduces a BuildCommand and its parser in the appropriate folder.
- Adds dedicated parsers for "dotnet add package" and "dotnet add reference" commands.
- Reorganizes the AddCommandParser to delegate to the new subcommand parsers.
Reviewed Changes
Copilot reviewed 962 out of 977 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/Cli/dotnet/commands/Build/BuildCommand.cs | Implements the BuildCommand using the new folder structure. |
src/Cli/dotnet/commands/Add/dotnet-add-package/AddPackageParser.cs | Sets up the command for adding packages. |
src/Cli/dotnet/commands/Add/dotnet-add-reference/AddProjectToProjectReferenceParser.cs | Handles the addition of project-to-project references. |
src/Cli/dotnet/commands/Build/BuildCommandParser.cs | Provides a parser for the build command with relevant options. |
src/Cli/dotnet/commands/Add/AddCommandParser.cs | Reorganizes the add command structure by delegating to the new subcommands. |
Files not reviewed (15)
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.tr.xlf: Language not supported
- src/Cli/dotnet/commands/Add/LocalizableStrings.resx: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.fr.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.ja.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.zh-Hant.xlf: Language not supported
- src/Cli/dotnet/commands/Build/LocalizableStrings.resx: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.ru.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.it.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.pl.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.zh-Hans.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.ko.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.pt-BR.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.cs.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.de.xlf: Language not supported
- src/Cli/dotnet/commands/Add/xlf/LocalizableStrings.es.xlf: Language not supported
If there are any concerns with individual folders or their names, I can handle that in a follow-up PR. I simply need to get the overall structure in place before I start tweaking things individually. |
<EmbeddedResource Update="**\*.resx" GenerateSource="true" /> | ||
<EmbeddedResource Update="*.resx" Namespace="Microsoft.DotNet.Cli" /> | ||
<EmbeddedResource Update="BuildServer\*.resx" Namespace="Microsoft.DotNet.Cli.BuildServer" /> | ||
<EmbeddedResource Update="CommandFactory\*.resx" Namespace="Microsoft.DotNet.Cli.CommandFactory" /> | ||
<EmbeddedResource Update="**\dotnet-package\add\*.resx" Namespace="Microsoft.DotNet.Tools.Package.Add" /> |
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 **
were overly "grabby" and would grab other folders in the project since the names are no longer unique. So, I changed them to be a more direct path to the .resx files. Otherwise, you'd get an error:
Two or more source files to be translated in the same project are named LocalizableStrings.resx. Give them unique names or set unique XlfTranslatedFilename metadata.
This was caused by the namespace for two different .resx files being updated to the same thing.
Changes look good, but is there a specific reason for uppercasing the folder names? Wouldn't this eventually be an issue on case-sensitive systems? I don't have a strong prefence, just curious. Update: I am guessing because of multiword commands and tooling not playing nicely with hyphens (?) |
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.
This looks correct. My only feedback is that I'd prefer x over X, i.e., workload instead of Workload, add instead of Add, etc.
What is the motivation for this change, to simplify it since they are all sub commands of dotnet as the root command, so it's redundant? |
@edvilme @Forgind @nagilson This isn't meant to sound derogatory in any way, but are y'all aware of what the standards are for scaffolding .NET projects? Meaning, it is extremely rare to have any lowercase folder names for folders that represent a .NET namespace. Just take a peek at the roslyn repo as they tend to follow the standards fairly closely. The intention is to make us follow the standards for .NET as we're the .NET SDK. I'll be updating the Again, these clearly aren't requirements (they're conventions) as we've not followed them before now, but I'd like us to get closer to an "ideal representation" for .NET repos, but we're quite a ways off from getting as consistent as a repo like roslyn. I believe that's what @baronfel is often advocating. |
Folder names following the dotnet commands works better for me because that's how I think of each folder: build holds files that have to do with dotnet build. I also find it easier to navigate around on linux if I don't have to alternate between typing capital letters and lowercase letters. I do think namespaces should have capitals, however. That said, if you say this is standard practice, I'm willing to fold. Note that I do think we have more of a reason to use lowercase letters than normal (and it's my apparently non-standard opinion that everyone should use camelCase or snake_case for folders) since the names for our folders come from something that's lowercase, but it's not wrong to want to follow what's considered standard. |
Summary
This is phase one of... many. I'll be rearranging/renaming things prior to doing more actual code changes. Here, this change ONLY moves the commands into folders that match the command name. No renaming of individual files or changes to namespaces yet.
Before -> After
dotnet-add
->Add
dotnet-build
->Build
dotnet-buildserver
->BuildServer
dotnet-clean
->Clean
dotnet-complete
->Complete
dotnet-format
->Format
dotnet-fsi
->Fsi
dotnet-help
->Help
dotnet-internal-reportinstallsuccess
->InternalReportInstallSuccess
dotnet-list
->List
dotnet-msbuild
->MSBuild
dotnet-new
->New
dotnet-nuget
->NuGet
dotnet-pack
->Pack
dotnet-package
->Package
dotnet-project
->Project
dotnet-publish
->Publish
dotnet-reference
->Reference
dotnet-remove
->Remove
dotnet-restore
->Restore
dotnet-restore-projectjson
->RestoreProjectJson
dotnet-run
->Run
dotnet-sdk
->Sdk
dotnet-sln
->Solution
dotnet-store
->Store
dotnet-test
->Test
dotnet-tool
->Tool
dotnet-vstest
->VSTest
dotnet-workload
->Workload