Allow printing SCIP index as JSON

[keynmol]

Goals:

1. ✅ Adds a --json flag to print command
2. ✅ Updates CLI docs as they've grown out of date~

Stretch goals:

• Fix (?) the testing commands as it seems they're not running on CI (i.e. CLI docs are out of date) Done in Fix tests that weren't running and have grown out of sync #148

Test plan

• Add basic Go tests

[varungandhi-src]

Some overall thoughts:

1. Subcommands should ideally be verbs, not nouns. That's already the pattern with existing subcommands (thought not all of them follow this). Thoughts on making this an extra --format= flag to scip print instead?
2. What's the motivation behind this subcommand? Is the output meant to be "stable" for some definition of stable? Since it's JSON, it's not unreasonable that someone may try to consume it. (Not saying it's a bad idea, but the motivation should at least be documented somewhere in the code/commit message etc.)
3. Thoughts on skipping the pretty formatting? One can easily pipe the result through jq if pretty formatting is desired.

[varungandhi-src]

Q: Why is this PR changing these lines? I thought the earlier PR that verified the documentation should've fixed this... 🤔

[keynmol]

The tests only verify that the global CLI help text is valid, not for any of the subcommands :(

It won't be much of an issue to add extra checks in a separate PR.

[varungandhi-src]

Could you remove the unrelated changes from this PR? I can submit a PR tomorrow making the test more robust and fix this.

[keynmol]

Could you remove the unrelated changes from this PR?

IMO, this is not a codebase with ridiculous velocity where unrelated changes are a problem - ongoing small improvements that aren't at odds with main purpose of the PR shouldn't be penalised by extra effort of reverting and resubmitting.

There are many small things that can be improved for which there is no plan and no dedicated effort - with a small number of contributors we should aim to fix things as we go.

You are free to follow up with a test for this (or I can do this) but updating and fixing small things in the docs should be a welcome addition to any PR.

[varungandhi-src]

First of all, my comment was not a blocking one.

ongoing small improvements that aren't at odds with main purpose of the PR shouldn't be penalised by extra effort of reverting and resubmitting.

I'm not trying to "penalise" something here... Ideally, these would not be part of the same PR (or commit) anyways. Normal commit hygiene IMO dictates that you'd commit related stuff in a single commit and move unrelated commit into separate PRs before submitting PRs. Even it got bundled into the same PR, if it were in different commits, it'd be easy to revert and push.

That's regardless of codebase velocity etc. My point isn't about velocity, it's about completeness. With drive-by improvements that get mixed in with other PRs, it's less clear what still needs fixing vs what doesn't. E.g. here, if I hadn't flagged this, then the point about the test missing might not have gone unnoticed.

That said, I don't think it's a big deal to leave it in.

[keynmol]

It's a process worth discussing separately :) meanwhile, I added an issue so we don't forget: #152

[keynmol]

Subcommands should ideally be verbs, not nouns. That's already the pattern with existing subcommands (thought not all of them follow this). Thoughts on making this an extra --format= flag to scip print instead?

I think an even better option would be to add a --json flag to print subcommand, as it's a more intuitive and recommended way of controling output for CLI https://clig.dev/#output. TODO: remove subcommand, make it a --json flag

What's the motivation behind this subcommand? Is the output meant to be "stable" for some definition of stable? Since it's JSON, it's not unreasonable that someone may try to consume it. (Not saying it's a bad idea, but the motivation should at least be documented somewhere in the code/commit message etc.)

The motivation wouls be to provide a machine readable and ubiqutous output format, something amenable to other CLI tools like jq. I looked around and there doesnt' seem to be an obvious way to inspect protobuf files in a dynamic fashion, like what jq provides. And even if there is such a tool, it's not as ubiqutous as JSON anyways.

w.r.t. stability, bar some variations in how to output missing fields, my understanding is that protobuf -> json conversion is unversal? So we can advertise it as stable.

Thoughts on skipping the pretty formatting? One can easily pipe the result through jq if pretty formatting is desired.

Yeah, I agree here. Once this becomes --json flag on the print command, there won't be a meaningful way to control the output, so we can just assume people will use jq. TODO: remove pretty-printing

[keynmol]

urfave/cli doesn't support positional arguments (urfave/cli#1074)

So the only form in which the command works is scip print --json <file>, not scip print <file> --json

It's annoying, but an alternative would be to manually process the arguments, which IMO isn't worth it at this point.

[varungandhi-src]

It seems a little weird to me the CLI guidelines recommend separate --json and --plain flags rather than a single --format=(plain|json) flag, but it's not a big deal.