Feature request: ability to add comments in a wit-manifest.json

[mwachs5]

Frankly, the "why I am I overriding this package here but not here" is unclear when faced with a mess of commit hashes. I suppose one can go dig through git blame and on GitHub, but adding the ability to have comments can help explain the user's intention / actual dependencies on a repo better.

[richardxia]

This is a little bit tricky because the JSON file format does not support comments, and while other JSON-like serialization formats do (YAML, TOML, JSON5, etc.), they will require a separate parsing library. Wit at the moment only relies on the Python standard library, which comes with a JSON parser, and adding a third party library will mean that Wit itself will need to be installed in a more complex way.

One possibly solution that requires minimal code changes to wit would be to follow NPM's example and officially recommend that users add a "//" key in the JSON file that wit will never actually read or use. For example, we could allow the following:

[
    {
        "//": "This is a comment that will be preserved",
        "commit": "471dfa250b87a16a461f583e2b7eed69293cef02",
        "name": "soc-testsocket-sifive",
        "source": "git@github.com:sifive/soc-testsocket-sifive.git"
    }
]

@jackkoenig, what do you think?

[sistillson]

I like yaml, or json5 but they do complicate setting up a useable environment.

The lack of comments it problem with json that come up a lot, and the npm solution is a clever work around, but it's kind of a hack.

I think it would be a lot clearer, if we really want comments, to bite the bullet early and either

a) change format from json to something else (yaml is a nice choice because it's common because of it's use in kubernetes and it's a strict superset of json, so the old files don't have to change)

or

b) write a preprocessor to strip comment out of json files before passing them to json.

[jackkoenig]

Per the additional dependencies issue, my thought is just to include any extra libraries (must be permissive) in the distribution of Wit itself. Basically include them as git submodules (or maybe wit packages!) and then build a tarball with all sources in good locations. You won't be able to install by just cloning the repo anymore, but that's fine. This does of course rely on the dependencies being pure Python, which may be a naive assumption on my part.

[jackkoenig]

I'm also totally down with the NPM-style comment thing. That would be super easy to implement.

[mmjconolly]

It looks like the current behavior is that wit won't error-out if you have added the "//":"comment" style of comment in the manifest.

Would the intention be that it also preserves the comment with a dependency/package during git revision update?

[mwachs5]

It could be nice if “update-pkg” took a -m argument that would replace that comment content

On Thu, Jan 2, 2020 at 5:45 PM Matthew Conolly notifications@github.com wrote:

It looks like the current behavior is that wit won't error-out if you have added the "//":"comment" style of comment in the manifest.

Would the intention be that it also preserves the comment with a dependency/package during git revision update?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sifive/wit/issues/194?email_source=notifications&email_token=AEIIAJAG6TK7D4VBGGICSQ3Q32KCZA5CNFSM4J536PLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIABTOY#issuecomment-570431931, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIIAJB4EJWDWXUURGL7HMDQ32KCZANCNFSM4J536PLA .

-- Megan A. Wachs VP of Engineering | SiFive, Inc 1875 South Grant Street Suite 600 San Mateo, CA 94402 megan@sifive.com

[richardxia]

It looks like the current behavior is that wit won't error-out if you have added the "//":"comment" style of comment in the manifest.

Would the intention be that it also preserves the comment with a dependency/package during git revision update?

Yes, I think it'd be good to explicitly preserve that comment.

[jackkoenig]

Fixed by https://github.com/sifive/wit/pull/195