Skip to content

Conversation

@zivkan
Copy link
Member

@zivkan zivkan commented Oct 23, 2024

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 nuget commands 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 why requiring 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 for dotnet nuget why, then we'd need to provide a way for the template to generate this and then opt the why argument into it:

CustomParser = ar =>
{
if (ar.Tokens.Count > 1)
{
var value = ar.Tokens[0];
ar.OnlyTake(1);
return value.Value;
}
ar.OnlyTake(0);
var currentDirectory = Directory.GetCurrentDirectory();
return currentDirectory;

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 config and dotnet nuget why were 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

  • Meaningful title, helpful description and 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

@zivkan zivkan requested a review from a team as a code owner October 23, 2024 23:24
Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of code generation as a tool to quickly get boilerplate code up and running. However, your description convinces me that this doesn't belong in our repo, and in general isn't being used. Could it be added to Entropy so that it can be ran as a tool that can be used at the developer's discretion?

@zivkan zivkan self-assigned this Oct 25, 2024
@zivkan
Copy link
Member Author

zivkan commented Oct 28, 2024

Could it be added to Entropy so that it can be ran as a tool that can be used at the developer's discretion?

I'll follow up with you directly to come up with a plan

@zivkan zivkan merged commit 104dc44 into dev Oct 28, 2024
@zivkan zivkan deleted the dev-zivkan-xplat-remove-tt-templates branch October 28, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants