-
Notifications
You must be signed in to change notification settings - Fork 385
add license as a possible field for opam list
#6841
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
base: master
Are you sure you want to change the base?
add license as a possible field for opam list
#6841
Conversation
|
couldn't run the tests locally (i think it requires some kind of set up) but I did run the compiled binary with the new flag and it worked |
kit-ty-kate
left a comment
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 for your contribution. Would you have time to add some tests? (format is documented in https://github.com/ocaml/opam/blob/master/tests/reftests/readme.md) If not we'll do it whenever we have some time
|
I had forgotten to push the test file, doing so now |
|
Looking at this change once again i'm realizing that it is already possible print the exact same thing using the raw field syntax: This isn't ideal and has been discussed in the past (#3664). We could at least document it in the man section for |
|
I didn't know about the raw-field syntax. It should be documented in the man/help I think. I can add that. Also the inconsistency between |
|
Added a specific mention in the doc. I realised it was mentioned before but this one is more visible. |
kit-ty-kate
left a comment
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.
modulo the PR title that needs to be changed, lgtm
| mk_opt ~cli cli_original ["columns"] "COLUMNS" ~section | ||
| (Printf.sprintf "Select the columns to display among: %s.\n\ | ||
| The special form $(b,<field>:) (field name followed by \ | ||
| colon, e.g., $(b,license:)) selects an arbitraty field. \ |
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.
| colon, e.g., $(b,license:)) selects an arbitraty field. \ | |
| colon, e.g., $(b,license:)) selects an arbitrary field. \ |
|
ah my bad i looked only at the new changes, the rest of the PR should probably require a longer discussion. The master_changes.md entries should also be updated (doc + test). I can do the last rebase at the end if needed |
|
It worth also adding a full test for |
|
Nice! Just for the record, this is what moby/vpnkit (from Docker) uses to autogenerate the licenses we ship with Docker; https://github.com/moby/vpnkit/blob/master/dune#L12 -- the raw field syntax is fine, but we can upgrade to this once released. |
Please update
master_changes.mdfile with your changes.