Skip to content
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

Spec for machine readable json output for dotnet list package #11446

Merged
merged 23 commits into from
Feb 23, 2022

Conversation

erdembayar
Copy link
Contributor

@erdembayar erdembayar commented Dec 9, 2021

Bug

Fixes: #11449
Related: #7752

Description

Many organization are required by regulation to audit packages that they're using in repository. It is difficult to parse the output of dotnet list package. Without having a good way to parse the output of this command, it makes it difficult to structure the list of packages in a way that can be fed into another auditing system.
In this spec design proposal, I tried to address json output part, for parsable format output it needs own spec.
For this purpose, I added below 2 new options for all dotnet list package commands.

  • --format <FORMAT>
    <FORMAT> - Allowed values as part of spec is json. Also text is acceptable input value too, it'll just output current cli output. (In the future parseable, csv, yaml, xml could be candidates.)
dotnet list [<PROJECT>|<SOLUTION>] package [--config <SOURCE>]
    [--deprecated]
    [--framework <FRAMEWORK>] [--highest-minor] [--highest-patch]
    [--include-prerelease] [--include-transitive] [--interactive]
    [--outdated] [--source <SOURCE>] [-v|--verbosity <LEVEL>]
    [--vulnerable]
    [--format <FORMAT>]

dotnet list package -h|--help

Rendered view

To do:

  • Rename spec md file to DotnetListPackageMachineReadableJsonOutput.md
  • Get community input https://twitter.com/nuget/status/1470426335705239557
  • Get explicit approval from stake holders
  • packages.lock.json format looks very similar what we want, code/schema could be reused.
  • Single json schema

@erdembayar erdembayar force-pushed the dev-eryondon-machinereadable-json branch 4 times, most recently from 7fd2858 to a150cf9 Compare December 10, 2021 01:14
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

When this spec is merged, it will not fix (implement) the linked issue. Therefore, we don't want the linked issue to be closed when this spec is merged.

@@ -0,0 +1,797 @@
# Machine readable JSON output for dotnet list package
Copy link
Member

Choose a reason for hiding this comment

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

the filename should include something about dotnet list package, because someone browsing the folder will not have any context about what MachineReadableOutput is about, and will be reasonable to assume to applies to all of NuGet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thank you for your review.
I'll rename to DotnetListPackageMachineReadableJsonOutput.md before I merge once it's approved. Now changing the file name makes hard to keep track of PR comments.

Copy link
Member

Choose a reason for hiding this comment

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

Probably a good time to rename this doc now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, should I copy to proposed/2022 folder?

proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
> Text2Xml.Lib 1.1.2 1.1.2 1.1.4
```

### dotnet list package --outdated --json
Copy link
Member

Choose a reason for hiding this comment

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

Is there a customer benefit to having json output outdated and vulnerable separately? I think there is for text output, but not for json.

Do we believe that customers will want to check both, or only one of the two? If customers want both, with the current proposal they'll have to call dotnet list package twice and possibly merge the results if they want to handle packages that are both deprecated and contain known vulnerabilities. If they only want one, but we implement json output containing both vulnerability and deprecation info, it costs almost nothing for the customer to ignore the part of the json they're not interested in.

We shouldn't let existing implementation details decide, or even contribute significantly to customer UX design, but both vulnerable and deprecation information comes from the same nuget http protocol resource, so there's basically zero cost to outputting both.

I think there is a benefit to json output having an "online" and "offline" mode, so dotnet list package --format json can be fast, without making network calls. But I don't see what customer benefit there is to forcing customers who want both vulnerability and deprecation information in json format to need to call the command twice and possible implement their own logic in merging the results.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I would expect --outdated to just show more metadata like

"outdated": true

Having to run the command twice seems bad, the point is to give something readable and the user can process the output in whatever manner they want.

Copy link
Contributor

Choose a reason for hiding this comment

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

And the same with --deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a proposal of what the ideal experience should look like? I agree that most customers who use the json output will probably want both, but we also want the output to meet their expectations based on existing behavior.

Would be weird to have dotnet list package --outdated --json also output vulnerable packages if they only expect outdated packages.

Similarly, most customers will probably also want transitive dependencies to create an SBOM.

Personally, I think the dotnet cli needs an all up dotnet list package --all that shows all deprecated, vulnerable, and outdated top level and transitive packages.

Altneraitvely the behavior of --outdated and --deprecated could be additive rather than exclusive. That adds to the scope of this work though.

Copy link
Member

Choose a reason for hiding this comment

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

 {
          "id": "Microsoft.Extensions.Primitives",
          "requestedVersion": "[1.0.0, 5.0.0]",
          "resolvedVersion": "1.0.0",
          "latestVersion": "6.0.0",
          "deprecated": "true",
          "vulnerable": "details?",
        },

Should the existence of latestVersion imply outdated?

There is obv a world where the installed version is not the latest available though, so maybe that warrants the existence of both.

Copy link
Member

Choose a reason for hiding this comment

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

fwiw, I actually think that we should design one format for all combinations, however you should be able to scope the output if you wanted to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please refer to this comment: #7752 (comment)

Human-readable output is first and foremost. This should be covered with the current list package command, however we may have some improvements coming to the command to make this better to glance at.

Machine-readable output is second to make output easier to handle, especially complex lists this command can produce (especially with transitive dependencies included)

The default format should be human-readable. The specified format should be machine-readable.

We should reach out to Chet Husk to help us understand what would scale with other dotnet CLI commands before leaning on one side.

@chgill-MSFT That is something I originally proposed but sadly we never got to it. We will have a similar experience in https://github.com/NuGet/Home/blob/dotnet-audit/proposed/2021/DotNetAudit.md

Copy link
Contributor Author

@erdembayar erdembayar Feb 2, 2022

Choose a reason for hiding this comment

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

Won't fix, out of scope from MVP. The behavior of the command is separate from the json output. I created tracking issue. #11551
Also overlap with dotnet audit spec.


```

### dotnet list package --include-transitive --json
Copy link
Member

Choose a reason for hiding this comment

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

Is there a compelling customer reason why we wouldn't always include transitive packages in json output? Since "top level packages" and transitive packages are being proposed in different parts of the json, customers who only care about "top level packages" can just ignore the other part of the json.

fewer options == simpler for customers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say its a bad user experience to have different defaults depending on if you specify --format json or not. I do agree fewer options is simpler, but there's no --do-not-include-transitive option to opt out of it right?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should support all the same combinations we support today and the json sohuld output whatever the --format less command would.

The only thing that I would recommend is that the json output schema is one and the same regardless of the option.

Copy link
Contributor

Choose a reason for hiding this comment

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

The output command should simply display output for the context of the command. i.e. --include-transitive --format json would show them.

Copy link
Contributor Author

@erdembayar erdembayar Feb 2, 2022

Choose a reason for hiding this comment

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

@NuGet/nuget-client
Currently this question main stumbling block. Whether we do --include-transitive implicitly for json output or not?
Please give your opinion.
I don't have any strong opinion, I feel adding --include-transitive manually not really issue customer, I want to merge this PR soon.

Copy link
Contributor

@JonDouglas JonDouglas Feb 2, 2022

Choose a reason for hiding this comment

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

I do not believe you should include this implicitly. It defeats the purpose of the parameter itself even if the previous design was not ideal for user experience. Another issue to track including this by default in the various list commands & respective arguments might be better to track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From offline discussion I strongly agree with The behavior of the command is separate from the json output.
We should address it in separately, I don't find example of other application/tool with different defaults depending on output type, it set wrong example, hard to justify later.
I created separate tracking issue: #11550

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not believe you should include this implicitly.

The behavior of the command is separate from the json output.

I agree with these statements mainly because of this: "parameters": "--include-transitive",.
If there was no way to know by looking at this JSON file whether transitives were being included in the query, I'd ask for it to be made clear.

Copy link
Contributor Author

@erdembayar erdembayar Feb 3, 2022

Choose a reason for hiding this comment

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

I do not believe you should include this implicitly.

The behavior of the command is separate from the json output.

I agree with these statements mainly because of this: "parameters": "--include-transitive",. If there was no way to know by looking at this JSON file whether transitives were being included in the query, I'd ask for it to be made clear.

We already have tracking issue for make --include-transitive by default, also I don't want to add --include-transitive not included for this, it's responsibility of user check how tool works, read documentation before use.

proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
"--parsable" option needs separate spec.
* Currently license info is not emitted from any cli command, it could be quite useful, we should consider in the future.

## Prior Art
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, vswhere has a -format <type> option, where <type> can be text, json, xml, or csv. I'm not proposing we support anything other than json and text, I'm just saying there's prior art in being able to explicitly set the default output (-format text), in addition to changing the output format.

Also, both MSBuild and .NET's test runner (technically vstest, but the customer entry point is dotnet test, despite dotnet vstest also existing) support the concept of loggers, which allow multiple to be used at the same time, or if only one is being used, it allows the customer to choose what format is being used. It also allows customers to write their own loggers, so they can output to whatever format they want. But their design and use-case is quite different. I don't know if we want to consider it prior-art, but then provide justification as to why we're not doing it in the same way since those are both already built into the dotnet cli and we're "inventing" a different way that does not yet exist in the dotnet cli.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing user logger like msbuild is interesting idea. But wondering could we do next iteration in the future or have to think about it in current implementation? Unless we're exposing ton of public new api then we can do later.


### Compatibility

We start with `version 1`, as long as we don't remove or rename then it'll be backward compatible. In case [we change version](https://stackoverflow.com/a/13945074) just add new properties, keep old ones even it's not used.
Copy link
Member

Choose a reason for hiding this comment

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

We start with version 1, as long as we don't remove or rename then it'll be backward compatible.

In that case, should we provide a --json-version 1 or --output-version 1 option, so if we create a version 2 output, then customers who are writing scripts to parse the json from version 1 don't suddenly start breaking? Remember that customers using cloud hosted CI agents may not choose when the version of the dotnet sdk is upgraded. Ok, there is the dotnet-install.[ps1|sh] script, allowing customers to install specific versions they want, and Azure DevOps does have a UseDotNet task to select a specific version. Is our guidance that customers who want deterministic output, that they must also control which specific version of the dotnet sdk is being used, or should we give customers the ability to control which version NuGet outputs?

keep old ones even it's not used

That doesn't feel like a very honest definition of backwards compatibility. If customers write scripts to parse the json and use the property, then a new version of the dotnet cli that starts output empty values is going to break the customer script's business logic. Sure, from a schema point of view it's still there, but from a pragmatic point of view it's breaking.

Copy link
Contributor

Choose a reason for hiding this comment

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

Versioning the output seems odd to me, vswhere and npm ls don't have this. Do we need to? Let's just never break the schema...

Copy link
Member

Choose a reason for hiding this comment

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

In which case doing good design up-front is more important, and we should leave the design spec up for a longer period of time and encourage customers to provide feedback, not treat this is a "quick" customer sprint issue where we rush the design and implementation.

There are trade-offs either way, we have to choose which one is more important to us.

Copy link
Member

Choose a reason for hiding this comment

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

especially when half the team is on vacation at the moment

Copy link
Contributor Author

@erdembayar erdembayar Dec 10, 2021

Choose a reason for hiding this comment

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

In that case, should we provide a --json-version 1 or --output-version 1 option, so if we create a version 2 output, then customers who are writing scripts to parse the json from version 1 don't suddenly start breaking?

I'm favor of just having version. But not sure about --output-version 1 part, as long as we're additive we're good. Of course, I welcome more feedbacks. Implementing this is not really hard, but I feel we should focus on MVP. Could we add --output-version 1 option if ever need version 2 in the future?

Copy link
Contributor Author

@erdembayar erdembayar Feb 2, 2022

Choose a reason for hiding this comment

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

Unfortunately, there are not much other .net CLI experience to compare. Only 1 which output is dotnet counter but output is very simply key value pair.
By the way we already have explicit versioning in .nupkg.metadata (version2), packages.lock.jsons file (version 1), I don't see any reason we do something different in this case

Copy link
Contributor

@JonDouglas JonDouglas Feb 3, 2022

Choose a reason for hiding this comment

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

I meant in other ecosystems that list out dependency graphs. There have been some hinted already such as npm ls, go list, cargo tree, and more.

Copy link
Contributor Author

@erdembayar erdembayar Feb 3, 2022

Choose a reason for hiding this comment

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

I meant in other ecosystems that list out dependency graphs. There have been some hinted already such as npm ls, go list, cargo tree, and more.

I already added my observation of npm ls json into prior art section, it's quite simpler than ours and doesn't have versioning.
For below tools I never used before I'm just sharing what I have found from search engine.
go list looks like parseable output than json output.
cargo tree is more of graphic tool than readable output.
mvn dependency:list it produces parseable output.
mvn dependency:tree produces tree like output.

Copy link
Contributor Author

@erdembayar erdembayar Feb 3, 2022

Choose a reason for hiding this comment

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

In addition to our .nupkg.metadata, packages.lock.json, we have versioning dependencyGraph , asset files too.

Copy link
Contributor Author

@erdembayar erdembayar Feb 4, 2022

Choose a reason for hiding this comment

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

After team discussion we include versioning in schema and output versioning like below. Intention behind output versioning even though it's not common in other .net CLI experience is our CI build script is failing so many times changes made by others unannounced and it's quite disruptive to us. We feel it's good practice to have people to have stable and predictable CI operations/output is important since this is machine readable output, maybe it could become example for others to follow. This way once setup pipeline work predictably for quite long time, otherwise customer always have to keep eye on changes.

dotnet list package --format json --output-version 1
dotnet list package --format yaml --output-version 1

dotnet list package -h|--help
```

### dotnet list package
Copy link
Member

Choose a reason for hiding this comment

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

Consider using heading levels to signal document structure. You already have a 3rd level heading dotnet list package example earlier in the document (line 50) under the same 2nd level heading. Therefore, from a document structure point of view, this is either redundant or using a bad header title. I get that it's being done to have the json and text versions close to each other for easier comparison, but since almost the entire document is level 3 headings under the same level 2 heading, it's a bit harder to skim over the document and understand what the different "sections" of the design are.

Also, consider that GitHub makes all headers HTML anchors, so they can be deep linked. All the headers with the same title will have the same anchor, and therefore it will be impossible to deep link any except the first. It's a very minor issue, but I think it shows attention to detail if it's avoided.

proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
@erdembayar erdembayar marked this pull request as ready for review December 10, 2021 15:34
@erdembayar erdembayar requested a review from a team as a code owner December 10, 2021 15:34
}
```

### Compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel like we need this. The CLI --json output is much different than a versioned web API for example. It should be expected that with new versions of NuGet, there is the possibility of a breaking change in a major version release. We should try to be additive where possible, but it's very possible some of these things will change with the evolution of tooling.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree. Interactive tools can change output because they're designed for humans to read, and when it unexpectedly changes output, humans can adapt. Adding a json output makes it an API, even if it's being output from a CLI tool rather than a web API.

Also consider the fact that it's most likely going to be used in CI scripts, and the version of the .NET SDK installed on the CI agent might not be in control of the team whose product is being built, it might be controlled by an infrastructure team, or another company altogether (GitHub and Azure DevOps hosted agents for non-Microsoft employees).

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't see how this is beneficial. The SDK version should be used to infer compatibility, not some command specific output "version". I haven't seen this in other CLI experiences output either. Why should we do this?

Copy link
Contributor

@JonDouglas JonDouglas left a comment

Choose a reason for hiding this comment

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

Great start of a spec Erick! Good formatting, helpful understanding of the problem, and consistency of a schema! I've left most of my feedback in the PR for now as points to consider!

🎉

proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
> Text2Xml.Lib 1.1.2 1.1.2 1.1.4
```

### dotnet list package --outdated --json
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I would expect --outdated to just show more metadata like

"outdated": true

Having to run the command twice seems bad, the point is to give something readable and the user can process the output in whatever manner they want.

> Text2Xml.Lib 1.1.2 1.1.2 1.1.4
```

### dotnet list package --outdated --json
Copy link
Contributor

Choose a reason for hiding this comment

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

And the same with --deprecated


```

### dotnet list package --include-transitive --json
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say its a bad user experience to have different defaults depending on if you specify --format json or not. I do agree fewer options is simpler, but there's no --do-not-include-transitive option to opt out of it right?


### Compatibility

We start with `version 1`, as long as we don't remove or rename then it'll be backward compatible. In case [we change version](https://stackoverflow.com/a/13945074) just add new properties, keep old ones even it's not used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Versioning the output seems odd to me, vswhere and npm ls don't have this. Do we need to? Let's just never break the schema...

proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
> Text2Xml.Lib 1.1.2 1.1.2 1.1.4
```

### dotnet list package --outdated --json
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a proposal of what the ideal experience should look like? I agree that most customers who use the json output will probably want both, but we also want the output to meet their expectations based on existing behavior.

Would be weird to have dotnet list package --outdated --json also output vulnerable packages if they only expect outdated packages.

Similarly, most customers will probably also want transitive dependencies to create an SBOM.

Personally, I think the dotnet cli needs an all up dotnet list package --all that shows all deprecated, vulnerable, and outdated top level and transitive packages.

Altneraitvely the behavior of --outdated and --deprecated could be additive rather than exclusive. That adds to the scope of this work though.

@erdembayar erdembayar force-pushed the dev-eryondon-machinereadable-json branch from fff8fa3 to bc7186f Compare December 10, 2021 19:04
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
> Text2Xml.Lib 1.1.2 1.1.2 1.1.4
```

### dotnet list package --outdated --json
Copy link
Member

Choose a reason for hiding this comment

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

 {
          "id": "Microsoft.Extensions.Primitives",
          "requestedVersion": "[1.0.0, 5.0.0]",
          "resolvedVersion": "1.0.0",
          "latestVersion": "6.0.0",
          "deprecated": "true",
          "vulnerable": "details?",
        },

Should the existence of latestVersion imply outdated?

There is obv a world where the installed version is not the latest available though, so maybe that warrants the existence of both.

> Text2Xml.Lib 1.1.2 1.1.2 1.1.4
```

### dotnet list package --outdated --json
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, I actually think that we should design one format for all combinations, however you should be able to scope the output if you wanted to.

"id": "NuGet.Core",
"requestedVersion": "2.13.0",
"resolvedVersion": "2.13.0",
"reasons": ["Legacy"],
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make it this easy to mix with other commands, so maybe we'd call this deprecation reasons?

what do others think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me it's reasonable thing to do, changed to deprecationReasons

Choose a reason for hiding this comment

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

Would it maybe be better to group these into a deprecationInfo: {} object or similar? It seems odd to have a few keys that are necessarily related to each other but that aren't grouped into a more meaningful construct that associates them.

ie:

{
          "id": "NuGet.Core",
          "requestedVersion": "2.13.0",
          "resolvedVersion": "2.13.0",
-         "reasons": ["Legacy"],
-         "alternativePackage": null
+         "deprecationInfo": {
+           "reasons": ["Legacy"],
+           "alternativePackage": null
+        }
}


```

### dotnet list package --include-transitive --json
Copy link
Member

Choose a reason for hiding this comment

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

I think we should support all the same combinations we support today and the json sohuld output whatever the --format less command would.

The only thing that I would recommend is that the json output schema is one and the same regardless of the option.

proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
@erdembayar
Copy link
Contributor Author

erdembayar commented Feb 2, 2022

@NuGet/nuget-client @aortiz-msft
Please review again. For concern and questions, I created follow up questions so we can address later.

@erdembayar erdembayar force-pushed the dev-eryondon-machinereadable-json branch from eb30928 to 95027ef Compare February 3, 2022 01:16
@erdembayar erdembayar force-pushed the dev-eryondon-machinereadable-json branch from 95027ef to b695020 Compare February 3, 2022 01:16
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
@erdembayar
Copy link
Contributor Author

@NuGet/nuget-client @JonDouglas @aortiz-msft
Addressed latest comments, please review again.

@jeffkl
Copy link
Contributor

jeffkl commented Feb 22, 2022

Team Review: @NuGet/nuget-client this is ready for final review

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks for driving this long effort!

A few comments on the spec (not necessarily the experience).

@@ -0,0 +1,797 @@
# Machine readable JSON output for dotnet list package
Copy link
Member

Choose a reason for hiding this comment

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

Probably a good time to rename this doc now :)

proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
proposed/2021/MachineReadableOutput.md Outdated Show resolved Hide resolved
Comment on lines 767 to 768
Currently, no other `dotnet command` implemented this, this is the 1st time dotnet command implementing `json`(etc) output, so it could become example for others next time they implement.
Please note, except "tab completion" (for dotnet) part all changes would be inside NuGet.Client repo(under NuGet.Core), and risk of introducing regression is low.(`--format text` refactoring related changes only come into my mind.), no impact on dotnet sdk.

Choose a reason for hiding this comment

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

I appreciate the time and thought for tab-completion and SDK impact! And many kudos for bringing us in on the ground floor on this effort as well.

<!-- Explain the proposal as if it were already implemented and you're teaching it to another person. -->
<!-- Introduce new concepts, functional designs with real life examples, and low-fidelity mockups or pseudocode to show how this proposal would look. -->

#### `--format` option
Copy link

@baronfel baronfel Feb 23, 2022

Choose a reason for hiding this comment

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

It would be ideal to have a 'short form' for --format, but -f is off the table because it belongs to force by broad convention and --framework in the .NET space specifically. I'll think on this more and short forms can be added easily later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, once we get MVP then we can polish it based on community input.

@erdembayar
Copy link
Contributor Author

@baronfel
Thank you for your review. I addressed your latest PR comments, please review again.

Copy link
Contributor

@JonDouglas JonDouglas left a comment

Choose a reason for hiding this comment

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

The spec with regards to --format is good with me. Although I don't necessarily agree with having an output version property, let's move forward. Great job Erick and everyone!

@baronfel
Copy link

@erdembayar your most recent changes look good to me. Happy to see this moving forward :)

@erdembayar
Copy link
Contributor Author

Since I have 3 approvals and waited for 3 months of feedbacks, I believe now it's time to merge spec.

@erdembayar erdembayar merged commit be38a0d into dev Feb 23, 2022
@erdembayar erdembayar deleted the dev-eryondon-machinereadable-json branch February 23, 2022 22:52
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.

Create spec for machine readable json output for "dotnet list package"