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 --format to dotnet sln list #39801
base: main
Are you sure you want to change the base?
Add --format to dotnet sln list #39801
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.
Left a single design note, but I'm very excited to see this! Thank you for taking the time to contribute this work.
src/Cli/dotnet/commands/dotnet-sln/list/SlnListReportOutputFormat.cs
Outdated
Show resolved
Hide resolved
console_with_header} and replace with SlnListReportOutputFormat.text
|
||
namespace Microsoft.DotNet.Tools.Sln.List | ||
{ | ||
internal class ListProjectsInSolutionCommand : CommandBase | ||
{ | ||
private readonly static JsonSerializerOptions s_noEscapeJsonSerializerOptions = new() { Encoder = System.Text.Encodings.Web.JavaScriptEncoder.UnsafeRelaxedJsonEscaping }; |
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.
Out of curiosity, why use this encoder specifically?
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 got confused and thought that this was what dotnet list package
used, but seems I ended up using what dotnet tool list
uses. I
s there a different one that is more common/would not pique your curiosity? :P
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 don't actually know what they use (the implementation is over in NuGet.Client repo I suppose, I could go look), but if you aligned with there decision I'm 100% ok with using that!
Don't worry about these - there's an out-of-band process that ships the untranslated entries to our localization team, they make adjustments, and then they send those changes back to us in a PR. |
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.
A test should be added for this. Likely, one that runs the command with --format
and then compares the output to see if it is correct.
text = 0, | ||
json = 1, |
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.
Enum members should be PascalCase, so Text
and Json
.
Add a
--format
option todotnet sln list
.Closes #26303
Unresolved questions:
console_with_header
with, or are there better names for this?console_with_header
is the default value?