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

Add option to print dependendies as JSON #4424

Merged
merged 8 commits into from Jun 26, 2019

Conversation

akshaymankar
Copy link
Contributor

@akshaymankar akshaymankar commented Nov 30, 2018

[Fixes #4101]

Format of the JSON:

[
    {
        "name": "transformers",
        "version": "0.5.2.0",
        "license": "BSD3"
    }
]

Doesn't print URL of source code yet. But I'd like to get some feedback on the format of the JSON.

Adding option to print yaml should be straightforward as Data.Yaml can print instances of ToJSON.

Another thing I'd like to discuss is, adding --json to options like --tree, --license, --separator is getting a bit confusing, as none of the options affect json printing. I was thinking maybe it would be better if we created another option like --format which could be text, tree, json and the --license and --separator would only be application to text and tree and we could codify that in the opts type to make sure the user gets error if they provide --license or --separator when format is json.

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

@mihaimaruseac
Copy link
Contributor

@mihaimaruseac mihaimaruseac commented Nov 30, 2018

Another thing I'd like to discuss is, adding --json to options like --tree, --license, --separator is getting a bit confusing, as none of the options affect json printing. I was thinking maybe it would be better if we created another option like --format which could be text, tree, json and the --license and --separator would only be application to text and tree and we could codify that in the opts type to make sure the user gets error if they provide --license or --separator when format is json

I think this proposal has sense and we should go that way.

Format of JSON seems ok to me.

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Dec 6, 2018

I tried a few things for the --format proposal, and i don't like something or other about each one of them. I have my least worst choice but it'd be great if somebody could comment on it. I am open to more ideas, my optparse foo is not as good as I'd have liked :)

So here are my attempts:

Attempt 1: Accept format as --format FORMAT and keep all fields as they are

This doesn't solve the problem of passing --licence or --separator with --format json. We'd have to fail the program using if else, which might get ugly with SPDX and YAML stuff. So, this is my least favourite option.

Attempt 2: Accept fields depending on what is passed to --format

This will solve the problem. But they way I could implement this was changing ListDepsOpts to look like:

data listDepsFormat = ListDepsFormatText { listDepsSep :: !Text, listDepsLicense :: !Bool }
                    | ListDepsFormatTree { listDepsSep :: !Text, listDepsLicense :: !Bool }
                    | ListDepsFormatJSON
data ListDepsOpts = { listDepsDotOpts :: !DotOpts
                    , listDepsFormat :: !ListDepsFormat}

This makes the help look like this:

Usage: stack ls dependencies (--json | --tree [--separator SEP] [--[no-]license] |
                             --text [--separator SEP] [--[no-]license])
                             [--[no-]external] [--[no-]include-base]
                             [--depth DEPTH] [--prune PACKAGES] [TARGET]
                             [--flag PACKAGE:[-]FLAG] [--test] [--bench]

Which is not nice as it looks very cluttered. Also --seperator and --[no]-license are printed twice in the Available options, this could be remedied by having only one of tree or text print the help, but then if you do --tree --help, it will not get printed. If somebody could help me solve these problems, this idea has potential to be the implementation.

Attempt 3: Accept different formats as sub-commands

This is so far my least worst option. The change made to ListDeptsOpts remains the same and the help text prints like this:

Usage: stack ls dependencies ([COMMAND] | [--separator SEP] [--[no-]license]
                             [--[no-]external] [--[no-]include-base]
                             [--depth DEPTH] [--prune PACKAGES] [TARGET]
                             [--flag PACKAGE:[-]FLAG] [--test] [--bench])
  View the dependencies

Available options:
  --separator SEP          Separator between package name and package
                           version. (default: " ")
  --[no-]license           Enable/disable printing of dependency licenses
                           instead of versions
  --[no-]external          Enable/disable inclusion of external dependencies
  --[no-]include-base      Enable/disable inclusion of dependencies on base
  --depth DEPTH            Limit the depth of dependency resolution (Default: No
                           limit)
  --prune PACKAGES         Prune each package name from the comma separated list
                           of package names PACKAGES
  TARGET                   If none specified, use all local packages. See
                           https://docs.haskellstack.org/en/v1.10.0/build_command/#target-syntax
                           for details.
  --flag PACKAGE:[-]FLAG   Override flags set in stack.yaml (applies to local
                           packages and extra-deps)
  --test                   Consider dependencies of test components
  --bench                  Consider dependencies of benchmark components
  -h,--help                Show this help text

Available commands:
  text                     Print dependencies as text (default)
  tree                     Print dependencies as tree
  json                     Print dependencies as JSON

I kept the command optional to maintain backward compatibility.
This one solves most of the problems but I'm not so sure that the format should be a command. It sounds semantically wrong.

@dbaynard
Copy link
Contributor

@dbaynard dbaynard commented Dec 13, 2018

I'm not sure the benefit of pretty json outweighs the cost of the extra dependency. Perhaps you could use yaml rather than aeson-pretty?

Edit: to be clear, I mean keep the (machine-readable) aeson output but don't prettify it, and if we want to enhance this with a prettier output then we can use yaml.

@dbaynard
Copy link
Contributor

@dbaynard dbaynard commented Dec 13, 2018

I have my least worst choice but it'd be great if somebody could comment on it.

I'll getting there! Thanks for your patience.

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Dec 13, 2018

Makes sense, I will remove the prettyfication of json. I was going to add yaml anyways.

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Feb 5, 2019

Sorry for vanishing on this. I was finding it difficult to dedicate more time to this. I think I can carve out some time in this week and the next one. Hopefully, I'll be able to figure out source information of packages.

@mihaimaruseac
Copy link
Contributor

@mihaimaruseac mihaimaruseac commented Feb 5, 2019

Hopefully, I'll be able to figure out source information of packages.

See #4494 for some documentation

@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Apr 11, 2019

@akshaymankar Just reviewing open PRs. I'm unclear what next steps are here, are you still interested in pursuing this PR?

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Apr 11, 2019

I am waiting for @dbaynard to finish the refactoring as he mentioned here: #4561 (comment).

@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Apr 12, 2019

@akshaymankar I've merged #4561 if you'd like to continue with this PR.

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Jun 16, 2019

Hello,

I think this PR finally is in a reviewable state, so I will remove the [WIP] from it.

There is one this I am not sure of, I couldn't find any way to get location of global packages. So, their location just doesn't show up. I know they're on hackage, but I'm not sure how and if we should look that up and show that.

Also, unlike discussed here, the git dependencies also show up with subdir as there could be multiple of them in the same repo. If the subdir was not specified it just shows up as empty string. I was a little split between omitting it and keeping it as empty string, but I thought if this is for machines to read, maybe it would be easier if it was uniform, so empty string just means root of the repo. Let me know what y'all think.

PS: The style tests are failing because of something unrelated.

@akshaymankar akshaymankar changed the title [WIP] Add option to print dependendies as JSON Add option to print dependendies as JSON Jun 16, 2019
@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Jun 17, 2019

I won't have a chance to review this today, and probably not all week due to travel. Beforehand: can you merge in master to your branch to fix the CI failure?

Copy link
Contributor

@snoyberg snoyberg left a comment

LGTM, just one minor comment

src/Stack/Dot.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@snoyberg snoyberg left a comment

LGTM, thanks!

@snoyberg snoyberg merged commit c52eddf into commercialhaskell:master Jun 26, 2019
8 checks passed
@akshaymankar akshaymankar deleted the machine-readable-deps branch Jun 26, 2019
@sschuberth
Copy link

@sschuberth sschuberth commented Jun 26, 2019

Sorry to spoil the party, but IIUC how this is implemented, it does not really fix my issue #4481:

Another thing I'd like to discuss is, adding --json to options like --tree, --license, --separator is getting a bit confusing, as none of the options affect json printing.

Yes, --tree should affect JSON printing, IMO. The choice of --text vs. --json is one about the format, whereas the choice of the default flat list vs. --tree is one about the topology. So of course you can represent both flat and tree topologies in JSON format, and you should be able to freely combine one of --text or --json with one of --tree or the default flat list.

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Jun 26, 2019

While I agree that --tree is about topology, it seems a bit too much to have the JSON with all the information about version, license, location and dependencies repeated.
I was planning to further add a dependencies field on each package in the JSON as described in #4837 (comment). This would make the JSON always represent the topology. Hopefully that would fix your issue #4101.

@sschuberth
Copy link

@sschuberth sschuberth commented Jun 26, 2019

Note that you don't necessarily have to repeat all of that information in the tree. A tree entry could just be a "reference" in the form of "name:version", and in the end you could have a flat set of the full meta-data for the used packages, similar to this format.

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Aug 10, 2019

@sschuberth Can you please confirm if #4994 works for your usecase?

@sschuberth
Copy link

@sschuberth sschuberth commented Aug 11, 2019

Yes, that PR looks very useful to cover my use-case. Thanks a lot @akshaymankar!

Edit: However, there's one thing that still confuses me: The JSON-array seems to contain all (direct and transitive) dependencies in a flat list, and your change "just" makes each entry also list the names of its direct dependencies, respectively. However, what I'd like to see is a tree (if the --tree option is specified) in the sense that on the first level only direct dependencies are listed, on the second level all dependencies of direct dependencies are listed etc.

In other words, given the JSON-array there currently seems to be no way to find out whether an entry in that array is a direct dependency or a transitive dependency. Is that correct?

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Aug 11, 2019

The JSON array also contains the root package(s), so you can get the list of direct dependencies from that.

@sschuberth
Copy link

@sschuberth sschuberth commented Aug 11, 2019

Ah, thanks for the explanation, then all looks good to me!

Edit: Again one amendment: How do I know (only from looking at the JSON) which of the listed packages is the root package?

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Aug 11, 2019

Hmm, I think there is no straightforward way to know that. If you don't mind running another command, easiest way I can think is to run stack ls dependencies --no-external and that should print only the local packages.

@develop7
Copy link
Contributor

@develop7 develop7 commented Oct 17, 2019

Shame it didn't make it into recent Stack release. We, the https://github.com/rikvdkleij/intellij-haskell users, could actually benefit from the feature, since there is way to export external dependencies' sources. I mean, the new CLI is even backward-compatible, isn't it?

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Oct 17, 2019

The change is not backward-compatible. I had to change ls dependencies --tree to ls dependencies tree. #4424 (comment) has more details.

@develop7
Copy link
Contributor

@develop7 develop7 commented Oct 17, 2019

Thank you for clarifying this for me, I've only skimmed this comment. Still, it feels like we could be fine with bit of clutter (attempt 2) if that'd be the price to have the feature sooner than later.
Also nice touch documenting the previous attempts.

@akshaymankar
Copy link
Contributor Author

@akshaymankar akshaymankar commented Oct 21, 2019

@develop7 Might be worth creating another issue. I don't think I will find time to work on this for now. Maybe somebody can add some backward compatibility so this can be released sooner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants