-
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
Add option for specifying the version of machine-readable output of dotnet list package command #29367
Add option for specifying the version of machine-readable output of dotnet list package command #29367
Conversation
@@ -63,6 +63,9 @@ internal static class ListPackageReferencesCommandParser | |||
public static readonly Option FormatOption = new ForwardedOption<ReportOutputFormat>("--format", LocalizableStrings.CmdFormatDescription) | |||
{ }.ForwardAsSingle(o => $"--format:{o}"); | |||
|
|||
public static readonly Option OutputVersiontOption = new ForwardedOption<int>("--output-version", LocalizableStrings.CmdOutputVersionDescription) |
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
It's currently just 1
or nothing option. I found it's bit challenging to add 1
as enum without changing other codes on how enum is parsed. So, I'm just using int
here.
@@ -171,4 +171,7 @@ | |||
<data name="CmdFormatDescription" xml:space="preserve"> | |||
<value>Specifies the output format type for the list packages command.</value> | |||
</data> | |||
<data name="CmdOutputVersionDescription" xml:space="preserve"> | |||
<value>Specifies the output version for the machine readable list packages command output.</value> |
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.
- From the title of the PR, I initially thought this option makes the output include the versions of the referenced packages, and wondered why they wouldn't be included by default.
- Is it necessary to mention "list packages command" in the option descriptions? I'd expect these descriptions to be displayed only when the user already specified that command.
I'd prefer something like "Specifies the version of machine-readable output", to make it clear that this doesn't refer to versions of packages. Perhaps also mention that this is for --format=json
.
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 addressed your concerns, please check.
src/Cli/dotnet/commands/dotnet-list/dotnet-list-package/ListPackageReferencesCommandParser.cs
Outdated
Show resolved
Hide resolved
…ckageReferencesCommandParser.cs Co-authored-by: Daniel Plaisted <daplaist@microsoft.com>
@@ -171,4 +171,7 @@ | |||
<data name="CmdFormatDescription" xml:space="preserve"> | |||
<value>Specifies the output format type for the list packages command.</value> | |||
</data> | |||
<data name="CmdOutputVersionDescription" xml:space="preserve"> |
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.
@dominoFire @nkolev92 @JonDouglas
Does wording look ok for you?
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 comment on string resources. Thank you!
src/Cli/dotnet/commands/dotnet-list/dotnet-list-package/LocalizableStrings.resx
Outdated
Show resolved
Hide resolved
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.
Signing-off.
@dsplaisted |
It will flow to main eventually. Once this PR is merged then there should be another one generated which will include these changes. |
Related to: NuGet/NuGet.Client#4855
Add new
--output-version
option for machine readable output ofdotnet list package
command. I forgot to include in #29017It would be used like below:
dotnet list package --format json --output-version 1