Stop using T4 templates to generate xplat CLI parser #6117
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug
Fixes: engineering (tech debt?)
Description
TL;DR I believe we'll make more progress porting all our commands to System.CommandLine by doing it manually, rather than trying to use a T4 template to generate the code.
The
dotnet nugetcommands currently shell out to NuGet.CommandLine.XPlat, which is a console app and therefore re-parses the CLI arguments and runs the command. Years ago, someone added a Commands.xml file that had the definitions of all the commands and options, and they also created T4 templates that generated all the C# code to implement those command and option definitions in the CLI parser we were using.In theory, this should make it "easy" to convert to a different CLI parser (and we need to migrate to System.CommandLine). However, a former team member spent a lot of effort doing this and did not complete it. I believe it's been a year (at least) since any work was done on this effort.
I'm not familiar with the T4 language, and I believe it will be less effort to manually convert each command, rather than trying to make the template do it. But there are a bunch of other reasons I'm not a fan of Commands.xml and the T4 templates.
One problem with using a template is that it becomes harder to take advantage of new CLI parser features, or implement advanced scenarios because first have to 1. figure out how to implement the feature with the parser and then 2. extend your abstraction to provide hooks to use the feature and then finally 3. add the new command, or modify an existing command, that uses this new abstraction. Without using templates, you'd be finished at step 1. There'd be some "code duplication" if you want to use the feature across multiple commands, but we've modified commands so infrequently, I believe that the effort on the templates has outweighed any benefit we could get from them.
An example of templates causing more, not less, effort is the fix to
dotnet nuget whyrequiring a directory, solution, or project file to be provided as an argument, rather than implicitly using the current directory, like all other dotnet CLI commands do. If we used templating fordotnet nuget why, then we'd need to provide a way for the template to generate this and then opt the why argument into it:NuGet.Client/src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Why/WhyCommand.cs
Lines 40 to 51 in fac3557
Consider also that the dotnet CLI has many more commands that NuGet does, but the dotnet/sdk repo doesn't have any such templating to automate the C# code. If having a short DSL (domain specific language, in our case Commands.xml) and then generating the C# code for the parser was a game changer, I would have expected the dotnet/sdk repo to have done something like this and reaped more benefit than we would. While it's not conclusive evidence that templating the commands is not beneficial, I think it is evidence that it's not so bad to manually write all the parser code.
Finally, considering that
dotnet nuget configanddotnet nuget whywere added without using Commands.xml or the templates, it seems that either the team's knowledge of these templates are lacking, or the choice to manually write these commands with System.CommandLine without using the templates was considered more worthwhile.One benefit of the templates that I actually like is the auto-generated documentation. However, we haven't been keeping it up to date with changes to dotnet's official docs, which means that if we were to regenerate the docs from the template, we'd have to check all the docs changes we'd lose, then modify this repo so that the docs template keeps the changes. Plus every time in the future anyone modifies the docs, someone would need to also modify this repo to make sure that the next time the docs are updated from the template, the changes aren't lost.
It's great in theory, but we haven't been disciplined enough in practise. I think we're better off removing the templates and just live with the extra effort, which isn't really very impactful given how infrequently we add or modify commands.
PR Checklist
a linked NuGet/Home issue(this doesn't have customer impact)Added tests(no product code changed)Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.no product code changes