-
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
Enable workload search for users to find latest set versions #42652
Enable workload search for users to find latest set versions #42652
Conversation
src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchCommandParser.cs
Outdated
Show resolved
Hide resolved
src/Cli/dotnet/commands/dotnet-workload/search/SearchWorkloadSetsParser.cs
Outdated
Show resolved
Hide resolved
} | ||
else | ||
{ | ||
Reporter.WriteLine(string.Join(',', versions)); |
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.
probably need to add a space after the comma, so ", "
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 was a little split on this, actually. Visually, I certainly agree. The other format I created is clearly intended for parsing, and I thought making it a true csv would make this version easy for parsing in an alternate scheme. If you don't think that's necessary (because json already exists), I can do that.
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 see, in that case, maybe just add a comment saying that we don't want the space because we want to be CSV friendly so someone doesn't "fix" this in the future.
{ | ||
if (optionResult.GetValueOrDefault<int>() <= 0) | ||
{ | ||
throw new ArgumentException("The --take option must be positive."); |
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.
Does this need to be localized?
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.
Yeah, probably
@@ -34,10 +41,44 @@ public WorkloadSearchCommand( | |||
|
|||
_sdkVersion = creationResult.SdkVersion; | |||
_workloadResolver = creationResult.WorkloadResolver; | |||
|
|||
_numberOfWorkloadSetsToTake = result.GetValue(SearchWorkloadSetsParser.TakeOption); | |||
_workloadSetOutputFormat = result.GetValue(SearchWorkloadSetsParser.FormatOption); |
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.
It looks like we don't do any validation that this is a valid value. So if you have jsno
or something as a typo it will just output it as CSV.
I don't think CSV with no spaces is great as a default. If the default is supposed to be human-readable, I would suggest outputting it in text
format where each version is on a separate line. Eventually this might change to be a table.
We should think about what information we might want to include in the future and try to make sure the output formats can support that without breaking changes. So for the json output we may want each element to be a dictionary that has a workloadVersion
key/value, so that we can add additional data (such as a description of what's updated, or what version of something like XCode is supported).
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 asked @baronfel what format he'd want for json a couple weeks ago, and he said he just wanted a JSON array of version strings.
I do think there are other things users could potentially want to know, but the key things (from my perspective) are things we'd expect the user to ask for more precisely in a future command. As an example, it's very natural for the user to want to know what workload versions are part of this random version we just gave them, but then they'd run dotnet workload search version 9.0.103
or whatever it is, and they'd get that information.
I do think having it be parseable as a csv is useful, but since both you and joeloff commented on that, I can make that an option like json and have the default (or typos) put versions on their own lines.
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.
Yeah - for this command which is just answering the question "which workload set versions are available", a simple list/json array makes sense.
IMO @dsplaisted's idea is much more aligned with the other commands we have been discussing - "what's in workload set X?" and "what components are supported in X?".
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 do agree with
I don't think CSV with no spaces is great as a default. If the default is supposed to be human-readable, I would suggest outputting it in text format where each version is on a separate line. Eventually this might change to be a table.
though - @Forgind please capture screenshots/examples and put them in the PR description to help discussions here.
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 put a few examples of what it should look like in the description. (Note that I made csv its own thing.)
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.
Thanks - this was very helpful for me to see!
CSV is controversial - I would suggest not doing it in the way that you have ti ow, because CSV is often used not for singular values but for rows of columnar data with a header. For example, for this version-listing command I'd expect the following output for
because most CSV-parsers understand the following conventions:
For these reasons I would strongly suggest removing CSV output from in-scope for this command until we have a better strategy for outputting CSV all-up in the SDK. |
Updated the description with the current state + examples of how to munge the JSON forms into newlines in unix shell and powershell. This LTGM! |
0211566
to
89e26bc
Compare
Progress on #42367
This adds three new parameters to the dotnet workload list command: version, --take, and --format. If version is specified, it will print the most recent
--take
workload sets (within the feature band; default (or if you specify a value < 1) is 5) and output them in --format (which can be json or csv; default (unspecified or other) is to put each version on its own line).Converting the json output to lines/other formats
sh (with jq)
Powershell core