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

Support --json option in opam tree #5303

Merged
merged 3 commits into from
Jan 9, 2023
Merged

Conversation

cannorin
Copy link
Contributor

@cannorin cannorin commented Sep 28, 2022

Closes #5298.

This PR adds support for --json option in opam tree command.

Here is an example output: https://gist.github.com/cannorin/ae87f55695c38356a9b880e0418a7a12

Currently, the output JSON has the type Forest in the following TS definition:

interface Package {
  name: string;
  version: string;
}

interface DepsTree extends Package {
  dependencies: DepsNode[];
}

interface DepsNode extends DepsTree {
  satisfies: string | null;
  is_dup: boolean;
}

interface RevdepsTree extends Package {
  requirements: RevdepsNode[];
}

interface RevdepsNode extends RevdepsTree {
  demands: string | null;
  is_dup: boolean;
}

type Forest = DepsTree[] | RevdepsTree[]

We discuss the structure of the output JSON in #5298.

TODO:

  • Add tests
  • Update doc and man not needed
  • Update master_changes.md

@rjbou rjbou added the PR: WIP Not for merge at this stage label Sep 28, 2022
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
I've moved the test to json testfile, and added some commands to test (with error, with simulated)

tests/reftests/json.unix.test Show resolved Hide resolved
@rjbou rjbou self-requested a review September 30, 2022 15:30
@rjbou
Copy link
Collaborator

rjbou commented Oct 13, 2022

thoughts from dev meeting: we should add switch name into the json, no need to add error / simulated state

@cannorin

This comment was marked as outdated.

@cannorin
Copy link
Contributor Author

Oops I broke some other tests in json.unix.test and I'm fixing it now.

@cannorin
Copy link
Contributor Author

@rjbou All the other subcommands are using the default man for --json, so we don't have to provide man for opam tree --json, right? If so, I think it's safe to mark this PR as ready for review.

@rjbou rjbou marked this pull request as ready for review October 14, 2022 09:38
@rjbou rjbou added PR: WAITING FOR REVIEW and removed PR: WIP Not for merge at this stage labels Oct 14, 2022
@kit-ty-kate
Copy link
Member

@smorimoto Could you confirm on whether or not this is what you wanted?

@smorimoto
Copy link
Member

@kit-ty-kate Yes! That looks OK! Let's go!

@smorimoto
Copy link
Member

When will this be deployed?

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

LGTM!
Wondering on some wordings:

  • is_dup -> is_duplicate ?
  • demands or satisfies are they enough indicative ?

@cannorin
Copy link
Contributor Author

is_duplicate may be better.

I didn't come up with better terms than demands/satisfies...

@dra27
Copy link
Member

dra27 commented Dec 19, 2022

is_duplicate and satisfies seem good - might be clearer with the example to figure whether something other demands/requirements wanted (and this is all just fixing names - the fundamental layout looks fine)

@smorimoto
Copy link
Member

Updates?

@rjbou
Copy link
Collaborator

rjbou commented Jan 6, 2023

queued on #5405

@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Jan 6, 2023
@rjbou rjbou merged commit 35be164 into ocaml:master Jan 9, 2023
@rjbou
Copy link
Collaborator

rjbou commented Jan 9, 2023

Thanks!

@smorimoto
Copy link
Member

Thank you for everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: QUEUED Pending pull request, waiting for other work to be merged or closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opam tree JSON output for Opam Dependency Submission
5 participants