Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upMove to TOML for manifest and lock #119
Comments
sdboyer
added
the
feedback
label
Jan 23, 2017
sdboyer
added this to the Manifest and Lock committable milestone
Jan 23, 2017
sdboyer
referenced this issue
Jan 23, 2017
Closed
Move to the format we actually intend for the manifest and lock #26
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
kardianos
Jan 23, 2017
In what instances would users be expected to manually modify any of the files, manifest or lock?
kardianos
commented
Jan 23, 2017
|
In what instances would users be expected to manually modify any of the files, manifest or lock? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
freeformz
Jan 23, 2017
Contributor
FWIW: I don't think it should generally be necessary with the exception of adding comments or the possibility that there are some seldom used features that are (at least to start) implemented through directly modifying the manfiest file. That's my 2c anyway.
|
FWIW: I don't think it should generally be necessary with the exception of adding comments or the possibility that there are some seldom used features that are (at least to start) implemented through directly modifying the manfiest file. That's my 2c anyway. |
sdboyer
added
help wanted
and removed
feedback
labels
Jan 23, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jessfraz
Jan 23, 2017
Contributor
agreed, don't think there should be much reason other than comments to directly edit the manifest, but if there is we should think about if that is the user experience we desire
|
agreed, don't think there should be much reason other than comments to directly edit the manifest, but if there is we should think about if that is the user experience we desire |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
I vote for sticking with JSON.
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sdboyer
Jan 24, 2017
Member
In what instances would users be expected to manually modify any of the files, manifest or lock?
echo what @freeformz and @jessfraz said. it's also possible that, when we start looking at direct integration into the go toolchain, we have to trim the command set down.
echo what @freeformz and @jessfraz said. it's also possible that, when we start looking at direct integration into the go toolchain, we have to trim the command set down. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
kardianos
Jan 24, 2017
when we start looking at direct integration into the go toolchain, we have to trim the command set down.
@sdboyer Is there a design document for a go tool integration? Are there docs from internal discussions? Are you just hypothesizing?
kardianos
commented
Jan 24, 2017
@sdboyer Is there a design document for a go tool integration? Are there docs from internal discussions? Are you just hypothesizing? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
somewhere around here: #25 (comment) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sdboyer
Jan 24, 2017
Member
Is there a design document for a go tool integration?
Nope, there are no more design docs hiding anywhere.
Are there docs from internal discussions? Are you just hypothesizing?
I'm basing this on some email chains with @rsc. He set out some desired constraints for how he wants the overall go toolchain to work, and the first thought I had was, "it would be easier to meet these if we weren't trying to provide a command for ever possible manifest change." I shared that thought in an email about two weeks ago, and there's been no further discussion since. That's where we're at.
Nope, there are no more design docs hiding anywhere.
I'm basing this on some email chains with @rsc. He set out some desired constraints for how he wants the overall go toolchain to work, and the first thought I had was, "it would be easier to meet these if we weren't trying to provide a command for ever possible manifest change." I shared that thought in an email about two weeks ago, and there's been no further discussion since. That's where we're at. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
kevinburke
Jan 24, 2017
Funny, I wrote about this recently - this is out of left field, but the only Go parsers I found that preserved comments were for XML, HCL and Go source files themselves, and they are about equally difficult to work with. https://kev.inburke.com/kevin/more-comment-preserving-configuration-parsers/
Here's a parser that finds a version number in a Go source file and bumps it: https://github.com/shyp/bump_version. I've also written code to manipulate HCL files and it's a little finicky but the resulting file is recognizable. https://github.com/hashicorp/hcl
kevinburke
commented
Jan 24, 2017
•
|
Funny, I wrote about this recently - this is out of left field, but the only Go parsers I found that preserved comments were for XML, HCL and Go source files themselves, and they are about equally difficult to work with. https://kev.inburke.com/kevin/more-comment-preserving-configuration-parsers/ Here's a parser that finds a version number in a Go source file and bumps it: https://github.com/shyp/bump_version. I've also written code to manipulate HCL files and it's a little finicky but the resulting file is recognizable. https://github.com/hashicorp/hcl |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nathany
Jan 24, 2017
Contributor
There is also JSON5, which does allow comments, though I don't think it's anywhere near a standard.
Though I prefer the readability of TOML > YAML > JSON, another consideration is the licensing and CLA if dep is later incorporated into the go toolchain. As far as I understand, the TOML or YAML libraries would need to be contributed under the Google CLA or rewritten (with comment preservation as well). /cc @willnorris
|
There is also JSON5, which does allow comments, though I don't think it's anywhere near a standard. Though I prefer the readability of TOML > YAML > JSON, another consideration is the licensing and CLA if |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
kevinburke
Jan 24, 2017
I'll just put this out there: I'm available for hire as a contractor, and would love to work on a CLA-compliant, Go TOML parser that can preserve comments and allow AST manipulation, if anyone's interested. https://burke.services
kevinburke
commented
Jan 24, 2017
|
I'll just put this out there: I'm available for hire as a contractor, and would love to work on a CLA-compliant, Go TOML parser that can preserve comments and allow AST manipulation, if anyone's interested. https://burke.services |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
andrew
Jan 24, 2017
+1 for a non-executable format for manifest+lockfile, this will allow third party services (like https://libraries.io) to analyse the dependencies without needing to execute arbitrary code download from the internet.
Also YAML is basically a superset of JSON, so you could mix and match, but just because you can doesn't mean you should,
Aside from that, allowing comments to explain why you switched to a particular fork or pinned to a weird tag would be
andrew
commented
Jan 24, 2017
|
+1 for a non-executable format for manifest+lockfile, this will allow third party services (like https://libraries.io) to analyse the dependencies without needing to execute arbitrary code download from the internet. Also YAML is basically a superset of JSON, so you could mix and match, but just because you can doesn't mean you should, Aside from that, allowing comments to explain why you switched to a particular fork or pinned to a weird tag would be |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
kr
Jan 24, 2017
There is a format that supports arbitrary comments, with regeneration preserving the comment positions, is supported by the Go standard library, and has syntax familiar to all Go programmers: Go source code.
Imagine a manifest file (with no .go filename extension) in a subset of Go syntax:
dependencies = {
"github.com/Masterminds/semver": {
"branch": "2.x",
},
"github.com/Masterminds/vcs": {
"version": "^1.8.0",
},
"github.com/pkg/errors": {
"version": ">=0.8.0, <1.0.0",
},
"github.com/sdboyer/gps": {
"branch": "master",
},
}Something along these lines wouldn't be too hard to put through ParseExpr and get an AST out, which can then be modified and pretty-printed.
Is that too bizarre?
kr
commented
Jan 24, 2017
|
There is a format that supports arbitrary comments, with regeneration preserving the comment positions, is supported by the Go standard library, and has syntax familiar to all Go programmers: Go source code. Imagine a dependencies = {
"github.com/Masterminds/semver": {
"branch": "2.x",
},
"github.com/Masterminds/vcs": {
"version": "^1.8.0",
},
"github.com/pkg/errors": {
"version": ">=0.8.0, <1.0.0",
},
"github.com/sdboyer/gps": {
"branch": "master",
},
}Something along these lines wouldn't be too hard to put through ParseExpr and get an AST out, which can then be modified and pretty-printed. Is that too bizarre? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jessfraz
Jan 24, 2017
Contributor
|
Your level of bizarre reminds me of myself :)
…On Tue, Jan 24, 2017, 11:33 Keith Rarick ***@***.***> wrote:
There is a format that supports arbitrary comments, with regeneration
preserving the comment positions, is supported by the Go standard library,
and has syntax familiar to all Go programmers: Go source code.
Imagine a manifest file (with no .go filename extension) in a subset of
Go syntax:
dependencies = {
"github.com/Masterminds/semver": {
"branch": "2.x",
},
"github.com/Masterminds/vcs": {
"version": "^1.8.0",
},
"github.com/pkg/errors": {
"version": ">=0.8.0, <1.0.0",
},
"github.com/sdboyer/gps": {
"branch": "master",
},
}
Something along these lines wouldn't be too hard to put through ParseExpr
and get an AST out, which can then be modified and pretty-printed.
Is that too bizarre?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#119 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABYNbITdrboTAIPBES5QJg7-hwW-jZikks5rVlINgaJpZM4LrVlt>
.
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
pquerna
Jan 24, 2017
FWIW, I had the same issues with preserving comments in config files for a project, and ended up using HCL with some wrapper code for my use case.
pquerna
commented
Jan 24, 2017
|
FWIW, I had the same issues with preserving comments in config files for a project, and ended up using HCL with some wrapper code for my use case. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
freeformz
Jan 24, 2017
Contributor
@kr Something similar was discussed early on, here was my attempt at the time (which was a little more aggressive than yours)....
package main
// +build deps
import (
"deps"
)
var (
// RootPackagName of the Project
RootPackageName = "github.com/freeformz/tdeps"
// IgnoreTags that should be ignored when analyzing deps
IgnoreTags = []string{"appengine", "test"}
// Dependencies stated for the package tree starting at RootPackage
Dependencies = []deps.Dependency{
{
Name: "github.com/Sirupsen/logrus",
Version: ">= v0.9.0",
},
{
Name: "github.com/heroku/private",
Version: "1.0.0",
Location: "git-hub-enterprise.herokai.com/privatething",
},
}
// Lock of resolved dependencies which may be in vendor/
Lock = []deps.Dependency{
{
Name: "github.com/Sirupsen/logrus",
Version: "v0.9.0",
},
{
Name: "golang.org/x/net/context",
Version: "someVer",
},
{
Name: "golang.org/x/oauth2",
Version: "someVer",
},
{
Name: "golang.org/x/oauth2/internal",
Version: "someVer",
},
}
)I don't remember why we didn't go down this route, possibly because my example wasn't a limited subset, but trying to do way more.
I do think @kr suggestion would be interesting to explore.
|
@kr Something similar was discussed early on, here was my attempt at the time (which was a little more aggressive than yours).... package main
// +build deps
import (
"deps"
)
var (
// RootPackagName of the Project
RootPackageName = "github.com/freeformz/tdeps"
// IgnoreTags that should be ignored when analyzing deps
IgnoreTags = []string{"appengine", "test"}
// Dependencies stated for the package tree starting at RootPackage
Dependencies = []deps.Dependency{
{
Name: "github.com/Sirupsen/logrus",
Version: ">= v0.9.0",
},
{
Name: "github.com/heroku/private",
Version: "1.0.0",
Location: "git-hub-enterprise.herokai.com/privatething",
},
}
// Lock of resolved dependencies which may be in vendor/
Lock = []deps.Dependency{
{
Name: "github.com/Sirupsen/logrus",
Version: "v0.9.0",
},
{
Name: "golang.org/x/net/context",
Version: "someVer",
},
{
Name: "golang.org/x/oauth2",
Version: "someVer",
},
{
Name: "golang.org/x/oauth2/internal",
Version: "someVer",
},
}
)I don't remember why we didn't go down this route, possibly because my example wasn't a limited subset, but trying to do way more. I do think @kr suggestion would be interesting to explore. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
kardianos
Jan 24, 2017
@kr @freeformz I'm skeptical of that route, but if it were to be pursued, make those top level vars part of a type so the fields can be looked up / autocomplete type things can assist.
kardianos
commented
Jan 24, 2017
|
@kr @freeformz I'm skeptical of that route, but if it were to be pursued, make those top level vars part of a type so the fields can be looked up / autocomplete type things can assist. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
mitsuhiko
Jan 24, 2017
I hate toml but I must admit that the choice of Toml was good in Rust land. It diffs very well which makes resolving conflicts much nicer than a json file would allow.
mitsuhiko
commented
Jan 24, 2017
•
|
I hate toml but I must admit that the choice of Toml was good in Rust land. It diffs very well which makes resolving conflicts much nicer than a json file would allow. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
typeless
Jan 25, 2017
The composability with other existing tools does make JSON desirable, which also makes it easier to be 'scriptable' (with the help of jq for example).
Not sure if that really matters in the end though.
typeless
commented
Jan 25, 2017
|
The composability with other existing tools does make JSON desirable, which also makes it easier to be 'scriptable' (with the help of Not sure if that really matters in the end though. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
vasi
Jan 25, 2017
An awful but effective workaround I've used in the past: Just have a key that is specced to be used for comments, and nothing else. Eg:
dependencies:
github.com/Masterminds/semver:
comment: "2.x is incompatible for reasons XYZ"
branch: 1.xThis comment will clearly be preserved even if the structure is programmatically modified.
Caveats:
- Can get awkward if keys are reordered. But usually it's easier to preserve key order than to preserve comments.
- Only one comment is possible per dictionary. Extensions (
comment2,commentXYZ…) can be allowed, but that's ugly. - Comments can't be added to arrays, only dictionaries. Not a big problem in practice. Arrays are typically small, or contain dictionaries—either of which allows comments to be close to where they should be.
vasi
commented
Jan 25, 2017
|
An awful but effective workaround I've used in the past: Just have a key that is specced to be used for comments, and nothing else. Eg: dependencies:
github.com/Masterminds/semver:
comment: "2.x is incompatible for reasons XYZ"
branch: 1.xThis comment will clearly be preserved even if the structure is programmatically modified. Caveats:
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
4ad
Jan 25, 2017
Member
@kr I like your suggestion (although I have no interest in Go dependency management) because it would encourage and make it easy for other projects to use the same configuration language.
|
@kr I like your suggestion (although I have no interest in Go dependency management) because it would encourage and make it easy for other projects to use the same configuration language. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Kegsay
Jan 25, 2017
I would love to see TOML used instead of JSON or YAML.
Hand-rolling JSON files is incredibly fiddly: I find I always miss a , or a :. The lack of being able to justify your versions (as mentioned by the OP) are infuriating when working with package managers like NPM which use package.json. It does parse easily though.
YAML is a lovely format to write in, but extremely complicated to parse. I think devising a completely compliant YAML implementation is very tricky, and even if dep manages to use one, what about 3rd parties who want to parse these files?
TOML gets the balance right IMHO: easy to parse, easy to write and easy to read. The most popular TOML parser for Golang AFAIK is https://github.com/BurntSushi/toml - so perhaps @BurntSushi or @cespare may have some thoughts on adding comment preservation support? I'd be interested in helping out.
Kegsay
commented
Jan 25, 2017
|
I would love to see TOML used instead of JSON or YAML. Hand-rolling JSON files is incredibly fiddly: I find I always miss a YAML is a lovely format to write in, but extremely complicated to parse. I think devising a completely compliant YAML implementation is very tricky, and even if TOML gets the balance right IMHO: easy to parse, easy to write and easy to read. The most popular TOML parser for Golang AFAIK is https://github.com/BurntSushi/toml - so perhaps @BurntSushi or @cespare may have some thoughts on adding comment preservation support? I'd be interested in helping out. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
BurntSushi
Jan 25, 2017
@Kegsay I'd expect comment preserving transformations to be a pretty significant change. I haven't done well with keeping the TOML implementation up to date with the latest spec either. I would personally support a replacement fork (i.e., we retire or move BurntSushi/toml) if you found it useful to start with the existing code.
BurntSushi
commented
Jan 25, 2017
|
@Kegsay I'd expect comment preserving transformations to be a pretty significant change. I haven't done well with keeping the TOML implementation up to date with the latest spec either. I would personally support a replacement fork (i.e., we retire or move BurntSushi/toml) if you found it useful to start with the existing code. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jrick
Jan 25, 2017
It may be too late in the design phase to change something like this, but I am completely ok with dropping the requirement of being able to modify the manifest through the tool instead of just hand editing it. Especially if the manifest is in an easily readable and writable format such as TOML. If the manifest is never rewritten by the tool, then there's no need to make it preserve comments.
jrick
commented
Jan 25, 2017
|
It may be too late in the design phase to change something like this, but I am completely ok with dropping the requirement of being able to modify the manifest through the tool instead of just hand editing it. Especially if the manifest is in an easily readable and writable format such as TOML. If the manifest is never rewritten by the tool, then there's no need to make it preserve comments. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Kegsay
Jan 25, 2017
@BurntSushi thanks for the quick reply! I feel that your TOML parser is the most "battle-worn" out of the available parsers in that it superficially seems to have the most usage, and therefore has been tested in a large amount of real-world scenarios. For that reason, I'd be reluctant to do a complete rewrite unless the change was sufficiently large that you'd end up doing a rewrite in order to implement comment preservation. Given you wrote the parser, do you believe that we'd need to start from scratch?
Kegsay
commented
Jan 25, 2017
|
@BurntSushi thanks for the quick reply! I feel that your TOML parser is the most "battle-worn" out of the available parsers in that it superficially seems to have the most usage, and therefore has been tested in a large amount of real-world scenarios. For that reason, I'd be reluctant to do a complete rewrite unless the change was sufficiently large that you'd end up doing a rewrite in order to implement comment preservation. Given you wrote the parser, do you believe that we'd need to start from scratch? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
BurntSushi
Jan 25, 2017
Given you wrote the parser, do you believe that we'd need to start from scratch?
It's not just the parser that needs attention. It's also the printer too. You need some way to carry comments through both parsing and commenting. There may be other issues, like preserving the order of keys, but I can't quite remember. I can't really be any more specific than "significant change" as to what's required. Comments were given precisely zero thought other than dropping them at lexing time.
BurntSushi
commented
Jan 25, 2017
•
It's not just the parser that needs attention. It's also the printer too. You need some way to carry comments through both parsing and commenting. There may be other issues, like preserving the order of keys, but I can't quite remember. I can't really be any more specific than "significant change" as to what's required. Comments were given precisely zero thought other than dropping them at lexing time. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
groob
Jan 25, 2017
Contributor
Has anyone considered HCL yet? https://github.com/hashicorp/hcl
It's a bit like JSON but supports human readable comments.
|
Has anyone considered HCL yet? https://github.com/hashicorp/hcl |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jbrodriguez
Jan 25, 2017
dep is awesome !
FWIW, I think the current manifest syntax is too verbose.
if the comments requirement is dropped, I'd prefer something more minimalistic
Node (package.json)
"dependencies": {
"classnames": "2.2.5",
"font-awesome": "4.6.3",
"history": "3.0.0",
"isomorphic-fetch": "2.2.1",
"react": "15.2.1",
"react-dom": "15.2.1",
"react-router": "2.6.0",
"react-tree-menu": "jbrodriguez/react-tree-menu#683858e",
"reactorx": "0.6.5"
}
jbrodriguez
commented
Jan 25, 2017
•
|
dep is awesome ! FWIW, I think the current manifest syntax is too verbose. if the comments requirement is dropped, I'd prefer something more minimalistic Node (package.json) "dependencies": {
"classnames": "2.2.5",
"font-awesome": "4.6.3",
"history": "3.0.0",
"isomorphic-fetch": "2.2.1",
"react": "15.2.1",
"react-dom": "15.2.1",
"react-router": "2.6.0",
"react-tree-menu": "jbrodriguez/react-tree-menu#683858e",
"reactorx": "0.6.5"
} |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
cespare
Jan 25, 2017
Hi! I help maintain github.com/BurntSushi/toml and I use TOML in a fair number of projects. I also have a strong interest in Go dependency management (I even wrote a few tools in the past, though these days I just use govendor + the vendor directory).
I support using TOML as the configuration language for this tool, but if dep needs to be able to rewrite the config file while supporting comments it will make things hard.
I have experience writing a parser+formatter (in Go, for another language) that supports comments. Handling them requires coming up with a bunch of heuristics about how to group comments and attach them to AST nodes so that they will be preserved in sensible ways in the output.
You can also see all the logic in the go/* packages (everything gofmt uses) to implement similar heuristics.
The main issue here is that most languages (including TOML and, I think, YAML) don't define how comments fit into the AST structure. (Total off-topic aside: nim is the only language I'm aware of that does.) So you're left with writing a bunch of semi-arbitrary rules and you keep coming across corner cases that don't feel right to the end user (they still crop up in gofmt from time to time).
All that said, I'd like to push back a bit on the premise:
dep is constantly rewriting the manifest and lock files
In other two-file dependency management systems I've used (mainly Ruby's bundler, but I think cargo and others work this way as well), the manifest file is only touched by humans and the lock file is only generated by machine. These properties make this problem go away: the manifest is only ever parsed, not generated, so comments are left untouched, and the lock file doesn't contain comments and isn't altered by hand.
Is it really impossible to restrict this manifest rewriting in dep? Here are two ways to avoid regenerating the manifest:
- Only ever generate a manifest initially. Future dependency changes must be done by hand.
- Only ever append new dependencies to the manifest. For example, if the format looks like this:
Then it is easy to append a new dependency programmatically by adding a few lines while leaving the rest of the file untouched:
someglobalconfig = "blah" [[dep]] path = "github.com/fsnotify/fsnotify" branch = "master" [[dep]] # This is a comment. path = "golang.org/x/sys/unix" branch = "master" # some other comment
[[dep]] path = "github.com/kr/pty" branch = "master"
I've read the dep help text and played around with it briefly; I apologize if I missed some fundamental design reason why it must be able to fully rewrite the manifest.
Finally, as much as I like TOML, I think that avoiding the need to programmatically rewrite hand-written TOML is important enough that JSON may still be the better choice if manifest-rewriting is a hard requirement. Even just allowing for a single comment string attached to each dependency gets a lot of the value here:
{
"dependencies": {
"github.com/fsnotify/fsnotify": {
"comment": "we pinned this to a rev because of <link to bug>. remember to unpin when that's fixed!",
"commit": "abc123"
}
}
}Sorry for the wall of text. I will gladly pitch in to help out if TOML changes (or even a new TOML library) would be useful.
cespare
commented
Jan 25, 2017
•
|
Hi! I help maintain github.com/BurntSushi/toml and I use TOML in a fair number of projects. I also have a strong interest in Go dependency management (I even wrote a few tools in the past, though these days I just use govendor + the vendor directory). I support using TOML as the configuration language for this tool, but if dep needs to be able to rewrite the config file while supporting comments it will make things hard. I have experience writing a parser+formatter (in Go, for another language) that supports comments. Handling them requires coming up with a bunch of heuristics about how to group comments and attach them to AST nodes so that they will be preserved in sensible ways in the output. You can also see all the logic in the go/* packages (everything gofmt uses) to implement similar heuristics. The main issue here is that most languages (including TOML and, I think, YAML) don't define how comments fit into the AST structure. (Total off-topic aside: nim is the only language I'm aware of that does.) So you're left with writing a bunch of semi-arbitrary rules and you keep coming across corner cases that don't feel right to the end user (they still crop up in gofmt from time to time). All that said, I'd like to push back a bit on the premise:
In other two-file dependency management systems I've used (mainly Ruby's bundler, but I think cargo and others work this way as well), the manifest file is only touched by humans and the lock file is only generated by machine. These properties make this problem go away: the manifest is only ever parsed, not generated, so comments are left untouched, and the lock file doesn't contain comments and isn't altered by hand. Is it really impossible to restrict this manifest rewriting in dep? Here are two ways to avoid regenerating the manifest:
I've read the dep help text and played around with it briefly; I apologize if I missed some fundamental design reason why it must be able to fully rewrite the manifest. Finally, as much as I like TOML, I think that avoiding the need to programmatically rewrite hand-written TOML is important enough that JSON may still be the better choice if manifest-rewriting is a hard requirement. Even just allowing for a single comment string attached to each dependency gets a lot of the value here: {
"dependencies": {
"github.com/fsnotify/fsnotify": {
"comment": "we pinned this to a rev because of <link to bug>. remember to unpin when that's fixed!",
"commit": "abc123"
}
}
}Sorry for the wall of text. I will gladly pitch in to help out if TOML changes (or even a new TOML library) would be useful. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
seh
Jan 25, 2017
I agree with @cespare here: the manifest is my artifact; the lock file is derived from it. No tool should touch my manifest without me explicitly requesting that it do so as a convenience.
For most operations Glide works this way. When I decide to invoke an operation that might have it change my manifest, I have to manually restore the comments that it drops (not to mention reordering entries to match my subjective preference of what belongs where). Out of such suffering comes avoidance of those mutative operations in favor of manual editing.
seh
commented
Jan 25, 2017
|
I agree with @cespare here: the manifest is my artifact; the lock file is derived from it. No tool should touch my manifest without me explicitly requesting that it do so as a convenience. For most operations Glide works this way. When I decide to invoke an operation that might have it change my manifest, I have to manually restore the comments that it drops (not to mention reordering entries to match my subjective preference of what belongs where). Out of such suffering comes avoidance of those mutative operations in favor of manual editing. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
freeformz
Jan 25, 2017
Contributor
FWIW: dep only alters the manifest based on commands the user issues as a convenience, aside from dep init anyway which does some discovery of existing deps and writes out the initial manifest file. You could alter it manually and run dep ensure.
My personal opinion is that I'd much rather have the tool do the alteration when I run a command and the work to enact the alteration instead of having to edit a file and then run a command to enact the alteration.
|
FWIW: My personal opinion is that I'd much rather have the tool do the alteration when I run a command and the work to enact the alteration instead of having to edit a file and then run a command to enact the alteration. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
BurntSushi
Jan 25, 2017
In case it's not clear: I completely defer to @cespare with respect to the fate of BurntSushi/toml.
BurntSushi
commented
Jan 25, 2017
|
In case it's not clear: I completely defer to @cespare with respect to the fate of |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Kegsay
Jan 26, 2017
The problem with the JSON-style comments of the form:
{
"dependencies": {
"github.com/fsnotify/fsnotify": {
"comment": "we pinned this to a rev because of <link to bug>. remember to unpin when that's fixed!",
"commit": "abc123"
}
}
}is that it breaks down a lot the moment you want to have any kind of extended prose (you can't have literal new lines), and you need to escape characters (e.g "), making it rather frustrating to put anything more than a quick one-liner. One can argue that is a good thing since it encourages people to only put comments if they really have to, but discouraging documentation isn't a good thing IMO.
Kegsay
commented
Jan 26, 2017
|
The problem with the JSON-style comments of the form: {
"dependencies": {
"github.com/fsnotify/fsnotify": {
"comment": "we pinned this to a rev because of <link to bug>. remember to unpin when that's fixed!",
"commit": "abc123"
}
}
}is that it breaks down a lot the moment you want to have any kind of extended prose (you can't have literal new lines), and you need to escape characters (e.g |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
punya
Jan 26, 2017
If we used YAML but with a comment field as opposed to an actual comment, we'd get preservation for free as well as support for multi-line prose
dependencies:
"github.com/fsnotify/fsnotify":
comment: >
we pinned this to a rev because of <link to bug>.
remember to unpin when that's fixed!
commit: abc123The downside is that this will confuse people who are used to the native comment syntax.
punya
commented
Jan 26, 2017
|
If we used YAML but with a comment field as opposed to an actual comment, we'd get preservation for free as well as support for multi-line prose dependencies:
"github.com/fsnotify/fsnotify":
comment: >
we pinned this to a rev because of <link to bug>.
remember to unpin when that's fixed!
commit: abc123The downside is that this will confuse people who are used to the native comment syntax. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sdboyer
Jan 26, 2017
Member
catching up...
A new/custom format
I think it would be unwise to invent any new format for this purpose, a la @kr's proposal. Comment preservation is nice, but it's not required, and IMO is less important than any one of the following:
- This tool already involves creating a lot of new things that are essential to the problem. New things are risky. We should not make new things for non-essential problems.
- It's hard to predict who else might want to read this data for what purpose, but @andrew already showed up with a use case. Creating a custom format would make that harder to do.
- The information we need to keep in a manifest is easily expressed declaratively. Comment preservation isn't worth sacrificing that, or even the appearance of it, with a [pseudo]-imperative format.
Manifest r/w
Brought up by a few people, but quoting from @cespare:
In other two-file dependency management systems I've used (mainly Ruby's bundler, but I think cargo and others work this way as well), the manifest file is only touched by humans and the lock file is only generated by machine. These properties make this problem go away: the manifest is only ever parsed, not generated, so comments are left untouched, and the lock file doesn't contain comments and isn't altered by hand.
This is also my preferred approach. I even have a cute graphic more or less to this effect:

(humans to the left, machines to the right)
We opted for this approach, though, because the dominant opinion amongst those who showed up was to have the tool handle updating the files. And I can't really argue with @freeformz's point that it's clearly more of a pain to edit a file THEN run a command, rather than simply run a command. If we were to go back to hand-editing the file, though, then this would be mostly solved - init can generate the manifest to start with, and from there on, it's hand-modified.
...with one exception: we also want to nudge users towards semver, and specifically carat range constraints, as a sane default. Without that sane default, users who don't care will have to provide their own default, which I worry could lead us to a project ecosystem with lots of unnecessary version constraint conflicts. So, being able to write new deps to the manifest is pretty key...but as @cespare noted, is probably workable just by appending the information.
So, if we were to drop commands that basically mutate the manifest for you, we might be able to get away without comment preservation.
Comment preservation
Again, @cespare:
The main issue here is that most languages (including TOML and, I think, YAML) don't define how comments fit into the AST structure. [..] So you're left with writing a bunch of semi-arbitrary rules and you keep coming across corner cases that don't feel right to the end user (they still crop up in gofmt from time to time).
On the one hand, I'm loathe to "meh" at problems like this, and I don't have any sense of what the corner cases look like, or how absurd the unexpected outcomes can be. For our needs here, though, perhaps we'd be able to tolerate it? My thinking on that:
- We won't be encoding any real semantics within comments, so there's no functional need for precision.
- The only truly intended audience is other contributors who are looking at the file. There isn't even something like godoc; even if we do end up creating a registry, we can just show the manifest file in its entirety. So, if a comment gets misgrouped and moved around, then it's still fine as long as a human is able to sorta figure out what happened.
- Manifest changes generally tend to be small and come a few at a time. If the machine did something weird, the user (or reviewer) is more likely to notice it and take corrective action than with an oddity buried in some chunky diff of actual code.
|
catching up... A new/custom formatI think it would be unwise to invent any new format for this purpose, a la @kr's proposal. Comment preservation is nice, but it's not required, and IMO is less important than any one of the following:
Manifest r/wBrought up by a few people, but quoting from @cespare:
This is also my preferred approach. I even have a cute graphic more or less to this effect: We opted for this approach, though, because the dominant opinion amongst those who showed up was to have the tool handle updating the files. And I can't really argue with @freeformz's point that it's clearly more of a pain to edit a file THEN run a command, rather than simply run a command. If we were to go back to hand-editing the file, though, then this would be mostly solved - ...with one exception: we also want to nudge users towards semver, and specifically carat range constraints, as a sane default. Without that sane default, users who don't care will have to provide their own default, which I worry could lead us to a project ecosystem with lots of unnecessary version constraint conflicts. So, being able to write new deps to the manifest is pretty key...but as @cespare noted, is probably workable just by appending the information. So, if we were to drop commands that basically mutate the manifest for you, we might be able to get away without comment preservation. Comment preservationAgain, @cespare:
On the one hand, I'm loathe to "meh" at problems like this, and I don't have any sense of what the corner cases look like, or how absurd the unexpected outcomes can be. For our needs here, though, perhaps we'd be able to tolerate it? My thinking on that:
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
UkiahSmith
Mar 16, 2017
From the thread on why Cargo uses TOML.
They are discussing the complex nature of the YAML specification.
But if you're using a subset of YAML, you're not using YAML. You're using a custom, specific format, that's poorly specified.
Then go on to say.
As soon as you start subsetting YAML, you run into the same problem. If you use a tool that assumes one YAML feature is present when writing, but your subset doesn't accept it, then you can't use that tool to produce YAML that Cargo can read.
TOML covers many of the same use cases, but is much simpler. JSON is also simpler, but not a good format for hand-written config files. TOML seems like a decent happy medium; well specified, not too complex, reasonably familiar ...
I agree with this sentiment. I feel a format such as TOML or HCL is less complex, and could prevent unforeseen issues in the future.
I wish HCL had a more formal spec, and a larger community of support around it.
TOML is well supported in both library implementations and editor support. Making it more likely to be familiar to users who are new to the dep tool.
UkiahSmith
commented
Mar 16, 2017
|
From the thread on why Cargo uses TOML. They are discussing the complex nature of the YAML specification.
Then go on to say.
I agree with this sentiment. I feel a format such as TOML or HCL is less complex, and could prevent unforeseen issues in the future. I wish HCL had a more formal spec, and a larger community of support around it. TOML is well supported in both library implementations and editor support. Making it more likely to be familiar to users who are new to the dep tool. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
willfaught
Mar 16, 2017
willfaught
commented
Mar 16, 2017
•
|
Kegsay:
From http://www.yaml.org/spec/1.2/spec.html:
The primary objective of this revision is to bring YAML into compliance with JSON as an official subset.
It's technically a superset, at least as of v1.2.
Even if it wasn't, in most cases they are compatible, as you pointed out, so I think the point still stands.
…On Thu, Mar 16, 2017 at 7:30 AM Ukiah Smith ***@***.***> wrote:
From the thread on why Cargo uses TOML.
They are discussing the complex nature of the YAML specification.
<https://users.rust-lang.org/t/why-does-cargo-use-toml/3577/17>
But if you're using a subset of YAML, you're not using YAML. You're using
a custom, specific format, that's poorly specified.
Then go on to say.
As soon as you start subsetting YAML, you run into the same problem. If
you use a tool that assumes one YAML feature is present when writing, but
your subset doesn't accept it, then you can't use that tool to produce YAML
that Cargo can read.
TOML covers many of the same use cases, but is much simpler. JSON is also
simpler, but not a good format for hand-written config files. TOML seems
like a decent happy medium; well specified, not too complex, reasonably
familiar ...
I agree with this sentiment. I feel a format such as TOML or HCL is less
complex, and could prevent unforeseen issues in the future.
I wish HCL had a more formal spec, and a larger community of support
around it.
TOML is well supported in both library implementations
<https://github.com/toml-lang/toml#implementations> and editor support
<https://github.com/toml-lang/toml#editor-support>. Making it more likely
to be familiar to users who are new to the dep tool.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#119 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD5VhlNsZKLhNzvGPyILX-mXDZjD1sjks5rmUdugaJpZM4LrVlt>
.
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
azr
Mar 17, 2017
Can't it be a simple golang slice of struct ?
#file vendor/dep.go
package vendor
import "dep"
var Dependencies = dep.List{
dep.Basic{ Pkg: "golang.org/x/crypto", Branch: "master" },
dep.Basic{ Pkg: "golang.org/x/net", Tag: "v42" },
dep.Basic{ Pkg: "golang.org/x/http2", MajorVersion: 30 },
// etc ...
}
It can be dumped to json or just used as a package by dep
azr
commented
Mar 17, 2017
•
|
Can't it be a simple golang slice of struct ?
It can be dumped to json or just used as a package by dep |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sdboyer
Mar 17, 2017
Member
@azr no - please see earlier parts of the discussion related to using an actual .go file. Such an approach is not on the table.
|
@azr no - please see earlier parts of the discussion related to using an actual |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nathany
Mar 17, 2017
Contributor
@willfaught HCL is also compatible with JSON.
Is that an advantage or disadvantage? It would be great for a tool transitioning from one format to another, but for dep, it can be clean break.
Sure, people who are more familiar with JSON could use that syntax instead, but is that a good thing?
It makes the format more complex. There is more than one way to do it. There is an extra burden for developers needing to parse (in their heads) multiple formats of the same data.
Not to say that is a strike against YAML or HCL. Just not necessarily an advantage for this use case IMO.
|
@willfaught HCL is also compatible with JSON. Is that an advantage or disadvantage? It would be great for a tool transitioning from one format to another, but for dep, it can be clean break. Sure, people who are more familiar with JSON could use that syntax instead, but is that a good thing? It makes the format more complex. There is more than one way to do it. There is an extra burden for developers needing to parse (in their heads) multiple formats of the same data. Not to say that is a strike against YAML or HCL. Just not necessarily an advantage for this use case IMO. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
azr
Mar 17, 2017
@sdboyer Ah sorry, I only saw the first "kinda .go" proposal not the second one but I still think it was ruled out a bit fast, and with not such certainty.
And I think it's just being ruled out because its looks too creative/out of the box thinking.
As far as I can tell the arguments against a .go/executable file are:
it's new/different from what already exists, therefore it's dangerous
-> well, go is go and we would not have to JSON
it cannot be parsed by external tools, like https://libraries.io
-> a generated file could be
it's not scriptable at the moment
-> it will be
All additional tool could better the cool tools we already have to dev in go.
All the formatting tools could also play with this file/pkg and would only have to do go.
And to take @freeformz's example it could look not so bad:
...
Dependencies = []dep.Dependency{
{ "github.com/Sirupsen/logrus", ">= v0.9.0" },
{ "github.com/heroku/private", "someVer" },
}
Or even
...
Dependencies = dep.Map{
"github.com/Sirupsen/logrus": ">= v0.9.0",
"github.com/heroku/private": "someVer",
}
Every dep entry could also be typed and know how to upgrade a dependency as well. ( like for private repos )
Please, at least make it a probable solution !
azr
commented
Mar 17, 2017
•
|
@sdboyer Ah sorry, I only saw the first "kinda .go" proposal not the second one but I still think it was ruled out a bit fast, and with not such certainty.
-> well, go is go and we would not have to JSON
-> a generated file could be
-> it will be All additional tool could better the cool tools we already have to dev in go. And to take @freeformz's example it could look not so bad:
Or even
Every dep entry could also be typed and know how to upgrade a dependency as well. ( like for private repos ) Please, at least make it a probable solution ! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
I'm sorry, but no, it's been definitively ruled out. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
willfaught
Mar 17, 2017
HCL is also compatible with JSON.
Forgot about HCL. Added an HCL example (at least, to the best of my understanding) to my post above. :)
Is that an advantage or disadvantage? It would be great for a tool transitioning from one format to another, but for dep, it can be clean break.
It's an advantage. JSON is great for program reading and writing, but not ideal for humans. YAML is great for humans, but not great for program reading and writing. Supporting both with equivalent formats has the most flexibility, allowing both humans and programs to easily read and write the configuration. Most people get JSON and YAML fundamentals, so I don't see an issue with complexity. If you run across some "crazy" YAML, it's trivial enough to simplify it with a tool.
Sure, people who are more familiar with JSON could use that syntax instead, but is that a good thing?
Alternatively: "Sure, people could use one syntax only, but is that a good thing?" Can't you use that argument against anything?
It makes the format more complex. There is more than one way to do it. There is an extra burden for developers needing to parse (in their heads) multiple formats of the same data.
If, say, YAML were chosen, then it would be just one format—YAML—wouldn't it? Whether it's JSON or YAML or TOML, you're using just one library to read and write it, so I don't see any extra burden for developers there.
I don't really have a strong opinion except against TOML. I really don't like INI-like syntax with the bracketed headers, so it's just a taste thing I guess; but I don't see much downside to picking YAML over JSON.
Regarding preserving comments: it seems to be the case that most YAML libraries today don't preserve comments? If that's the case, then while it might be fine for the dep impl to use a library that does preserve them, it doesn't guarantee that other developers who write tools will use the same library. So those comments are getting lost anyway, whatever dep does. The only way to have useful preserved comments, as I see it, is to either pick a format where most tools support comment preservation, or make the comments part of the data schema. I don't think it's a big deal to support an extra comment attribute on stuff. Most of the time there won't be one anyway. Arguably a comment attribute could be useful for tools anyway.
willfaught
commented
Mar 17, 2017
•
Forgot about HCL. Added an HCL example (at least, to the best of my understanding) to my post above. :)
It's an advantage. JSON is great for program reading and writing, but not ideal for humans. YAML is great for humans, but not great for program reading and writing. Supporting both with equivalent formats has the most flexibility, allowing both humans and programs to easily read and write the configuration. Most people get JSON and YAML fundamentals, so I don't see an issue with complexity. If you run across some "crazy" YAML, it's trivial enough to simplify it with a tool.
Alternatively: "Sure, people could use one syntax only, but is that a good thing?" Can't you use that argument against anything?
If, say, YAML were chosen, then it would be just one format—YAML—wouldn't it? Whether it's JSON or YAML or TOML, you're using just one library to read and write it, so I don't see any extra burden for developers there. I don't really have a strong opinion except against TOML. I really don't like INI-like syntax with the bracketed headers, so it's just a taste thing I guess; but I don't see much downside to picking YAML over JSON. Regarding preserving comments: it seems to be the case that most YAML libraries today don't preserve comments? If that's the case, then while it might be fine for the dep impl to use a library that does preserve them, it doesn't guarantee that other developers who write tools will use the same library. So those comments are getting lost anyway, whatever dep does. The only way to have useful preserved comments, as I see it, is to either pick a format where most tools support comment preservation, or make the comments part of the data schema. I don't think it's a big deal to support an extra comment attribute on stuff. Most of the time there won't be one anyway. Arguably a comment attribute could be useful for tools anyway. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nathany
Mar 17, 2017
Contributor
@willfaught Thanks for adding the HCL example. I was having trouble coming up with what it would look like.
Installing and running https://github.com/fatih/hclfmt reported some errors until I quoted all the keys and values like so:
dependencies {
"github.com/gorilla/mux" = "^1.3.0"
"github.com/st3fan/gocrypto" = "master"
"golang.org/x/crypto" = "master"
"golang.org/x/net" = "master"
}
As @flibustenet pointed out, we should consider what multiple parameters would look like.
The following is valid HCL. The hclfmt tool will split it across multiple lines, which wouldn't be great for diffs, but maybe hclfmt could be changed.
// comment header
dependencies {
"github.com/gorilla/mux" = { version = "^1.3.0" }
"github.com/st3fan/gocrypto" = { branch = "master" }
"golang.org/x/crypto" = { branch = "master" }
"golang.org/x/net" = { branch = "master" }
}
Notice that this looks almost identical to the TOML version, except it nests dependencies in a map instead of having the special INI-style format for the same thing (tables vs. inline tables in TOML).
+ It's nice that HCL uses Go-style // comments
|
@willfaught Thanks for adding the HCL example. I was having trouble coming up with what it would look like. Installing and running https://github.com/fatih/hclfmt reported some errors until I quoted all the keys and values like so:
As @flibustenet pointed out, we should consider what multiple parameters would look like. The following is valid HCL. The
Notice that this looks almost identical to the TOML version, except it nests dependencies in a map instead of having the special INI-style format for the same thing (tables vs. inline tables in TOML).
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nathany
Mar 17, 2017
Contributor
YAML has an inline hash so it could look like this:
dependencies:
github.com/gorilla/mux: { version: ^1.3.0 }
github.com/st3fan/gocrypto: { branch: master }
golang.org/x/crypto: { branch: master }
golang.org/x/net: { branch: master }|
YAML has an inline hash so it could look like this: dependencies:
github.com/gorilla/mux: { version: ^1.3.0 }
github.com/st3fan/gocrypto: { branch: master }
golang.org/x/crypto: { branch: master }
golang.org/x/net: { branch: master } |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nathany
Mar 17, 2017
Contributor
@willfaught I agree that YAML has a nice aesthetic. The use of colons for key: value mappings is even reminiscent of composite literals.
Personally, any of HCL, TOML, YAML would be fine with me.
+ comments
+ one line per dependency for diffs and merging (JSON is adverse to extra trailing commas)
+ readable/ergonomics
YAML is the most mature, but there are a few strikes against it too -- in my opinion.
Significant Whitespace
"structure is determined by indentation" http://yaml.org/spec/1.2/spec.html#space/indentation/
I don't personally have a problem with significant whitespace, but I observe that Go intentionally doesn't rely on it.
"Some observers objected to Go's C-like block structure with braces, preferring the use of spaces for indentation, in the style of Python or Haskell. However, we have had extensive experience tracking down build and test failures caused by cross-language builds where a Python snippet embedded in another language, for instance through a SWIG invocation, is subtly and invisibly broken by a change in the indentation of the surrounding code. Our position is therefore that, although spaces for indentation is nice for small programs, it doesn't scale well, and the bigger and more heterogeneous the code base, the more trouble it can cause. It is better to forgo convenience for safety and dependability, so Go has brace-bounded blocks." https://talks.golang.org/2012/splash.article
Even if it turns out to be less of an issue for a dependency tool config file, a consistent stance does have some value IMO.
Complexity
Compare http://yaml.org/spec/1.2/spec.html to https://golang.org/ref/spec. YAML has way more than necessary for such a simple need. I do remember the remote code execution vulnerabilities the Ruby community experienced partially due to YAML. That may not be a problem for the Go parsers, but it was enough for the Rust (Cargo) folks to avoid the format.
|
@willfaught I agree that YAML has a nice aesthetic. The use of colons for Personally, any of HCL, TOML, YAML would be fine with me. YAML is the most mature, but there are a few strikes against it too -- in my opinion. Significant Whitespace
I don't personally have a problem with significant whitespace, but I observe that Go intentionally doesn't rely on it.
Even if it turns out to be less of an issue for a dependency tool config file, a consistent stance does have some value IMO. ComplexityCompare http://yaml.org/spec/1.2/spec.html to https://golang.org/ref/spec. YAML has way more than necessary for such a simple need. I do remember the remote code execution vulnerabilities the Ruby community experienced partially due to YAML. That may not be a problem for the Go parsers, but it was enough for the Rust (Cargo) folks to avoid the format. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
willfaught
Mar 18, 2017
willfaught
commented
Mar 18, 2017
|
Re: whitespace:
Code is different than config. It's copied, rewritten, transformed, edited,
etc. a lot. I don't think anyone would be working with dep config that way.
Do you foresee there being deep nesting in the config?
Re: complexity:
You don't need to read the full spec to learn YAML, and the YAML dep would
be writing would be pretty simple, I think.
My understanding of the YAML-related Rails security flaw was that Ruby code
was reading YAML values and "eval"-ing code with them. So someone basically
did an injection attack like you would for SQL. The same thing could happen
with reading from JSON.
…On Fri, Mar 17, 2017 at 3:48 PM Nathan Youngman ***@***.***> wrote:
@willfaught <https://github.com/willfaught> I agree that YAML has a nice
aesthetic. The use of colons for key: value mappings is even reminiscent
of composite literals.
Personally, any of HCL, TOML, YAML would be fine with me.
+ comments
+ one line per dependency for diffs and merging (JSON is adverse to extra
trailing commas)
+ readable/ergonomics
YAML is the most mature, but there are a few strikes against it too -- in
my opinion.
Significant Whitespace
"structure is determined by indentation"
http://yaml.org/spec/1.2/spec.html#space/indentation/
I don't personally have a problem with significant whitespace, but I
observe that Go intentionally doesn't rely on it.
"Some observers objected to Go's C-like block structure with braces,
preferring the use of spaces for indentation, in the style of Python or
Haskell. However, we have had extensive experience tracking down build and
test failures caused by cross-language builds where a Python snippet
embedded in another language, for instance through a SWIG invocation, is
subtly and invisibly broken by a change in the indentation of the
surrounding code. Our position is therefore that, although spaces for
indentation is nice for small programs, it doesn't scale well, and the
bigger and more heterogeneous the code base, the more trouble it can cause.
It is better to forgo convenience for safety and dependability, so Go has
brace-bounded blocks." https://talks.golang.org/2012/splash.article
Even if it turns out to be less of an issue for a dependency tool config
file, a consistent stance does have some value IMO.
Complexity
Compare http://yaml.org/spec/1.2/spec.html to https://golang.org/ref/spec.
YAML has way more than necessary for such a simple need. I do remember the
remote code execution vulnerabilities the Ruby community experienced
partially due to YAML. That may not be a problem for the Go parsers, but it
was enough for the Rust folks to avoid the format.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#119 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD5Vh8KY3sJ6PRT9f4Pwn7UAyi9f9wXks5rmw2_gaJpZM4LrVlt>
.
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sdboyer
Mar 20, 2017
Member
We're going to go with TOML.
As I think discussion of their various characteristics here has demonstrated, there's no clear winner between YAML, TOML, or HCL in the general case. I say "various characteristics" to emphasize that I'm not talking about personal views (my view: "all formats are terrible and I will hate them at least sometimes - life sucks, buy a helmet").
The one thing that does stick out with TOML is, being not tree-structured, it's possible for us to append constraints to the manifest without rewriting it. That may turn out to be a very important factor in applying sane defaults that help guard us (that is, the entire public Go ecosystem) against nasty exponential growth in solver running time.
This would be a great moment for anyone with time to jump in with a PR that converts us to use TOML for the manifest and lock. For now at least, let's rely on BurntSushi/toml.
EDIT: due to licensing concerns, we're gonna at least start from https://github.com/pelletier/go-toml.
|
We're going to go with TOML. As I think discussion of their various characteristics here has demonstrated, there's no clear winner between YAML, TOML, or HCL in the general case. I say "various characteristics" to emphasize that I'm not talking about personal views (my view: "all formats are terrible and I will hate them at least sometimes - life sucks, buy a helmet"). The one thing that does stick out with TOML is, being not tree-structured, it's possible for us to append constraints to the manifest without rewriting it. That may turn out to be a very important factor in applying sane defaults that help guard us (that is, the entire public Go ecosystem) against nasty exponential growth in solver running time. This would be a great moment for anyone with time to jump in with a PR that converts us to use TOML for the manifest and lock. For now at least, let's rely on BurntSushi/toml. EDIT: due to licensing concerns, we're gonna at least start from https://github.com/pelletier/go-toml. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
If it's okay, I have time this week to jump over to TOML. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
willfaught
Mar 20, 2017
willfaught
commented
Mar 20, 2017
|
Can't HCL also be non-structured?
foo bar {
baz = 42
}
etc. HCL at least can be both, but TOML can't:
foo bar {
baz = 42
boz {
foo = 101
}
}
You get the best of both worlds.
…On Mon, Mar 20, 2017 at 7:45 AM sam boyer ***@***.***> wrote:
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
@willfaught decision's made - let's move on, please. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
@carolynvs absolutely! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sdboyer
Mar 20, 2017
Member
Actually, let's maybe start from https://github.com/pelletier/go-toml to avoid dealing with the WTFPL right now. Or, another one. Honestly, idc, I'm just trying to make suggestions to break logjams before they happen
|
Actually, let's maybe start from https://github.com/pelletier/go-toml to avoid dealing with the WTFPL right now. Or, another one. Honestly, idc, I'm just trying to make suggestions to break logjams before they happen |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
carolynvs
Mar 20, 2017
Collaborator
I don't have strong feels on the TOML library, happy to use whichever you prefer. I've never run into the WTFPL though. go-toml a try first.
|
I don't have strong feels on the TOML library, happy to use whichever you prefer. I've never run into the WTFPL though. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
rucuriousyet
Mar 20, 2017
What about HCL? I know this has been mentioned before. Given that it has JSON interop, you could also allow certain JSON producing scripts as an alternative. For example, dep would recognize that the man file has a .go, .rb or .py extension. When those extensions are recognized, dep runs the file and collects the output as JSON etc.
This could be amazing for people who want to do something different with their project or simply want some logic determining what deps they are using.
dep "github.com/gin-gonic/gin" {
version = "1.1.0"
}
job "build-client" { ...
or
require 'go-dep'
def build do
puts dep.conf ...
rucuriousyet
commented
Mar 20, 2017
•
|
What about HCL? I know this has been mentioned before. Given that it has JSON interop, you could also allow certain JSON producing scripts as an alternative. For example, dep would recognize that the man file has a .go, .rb or .py extension. When those extensions are recognized, dep runs the file and collects the output as JSON etc. This could be amazing for people who want to do something different with their project or simply want some logic determining what deps they are using. dep "github.com/gin-gonic/gin" {
version = "1.1.0"
}
job "build-client" { ...
or require 'go-dep'
def build do
puts dep.conf ...
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
@rucuriousyet decision's made - let's move on, please. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
rucuriousyet
Mar 20, 2017
Please close the issue then... theres so much content here I didn't notice...
rucuriousyet
commented
Mar 20, 2017
|
Please close the issue then... theres so much content here I didn't notice... |
sdboyer
changed the title from
Manifest and Lock format: TOML/YAML/JSON
to
Move to TOML for manifest and lock
Mar 20, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
sdboyer
Mar 20, 2017
Member
Fair enough - I've updated the issue title and OP accordingly.
I'm keeping the issue open, though, because we still need to actually do it
|
Fair enough - I've updated the issue title and OP accordingly. I'm keeping the issue open, though, because we still need to actually do it |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
bep
Mar 20, 2017
For now at least, let's rely on BurntSushi/toml.
TOML is a great choice!
But I want to chime in that we switched from BurntSushi/toml to go-toml in some of the @spf13 projects at one point, after a big license debate. go-toml have had some issues, but the maintainer is pretty responsive, and the bugs seem to have been ironed out.
bep
commented
Mar 20, 2017
TOML is a great choice! But I want to chime in that we switched from |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
@bep it's good to get some direct feedback on that, thank you! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
tro3
Mar 20, 2017
Contributor
@carolynvs - I'm happy to help here, if you want to take the lead on the conversion. Just let me know.
|
@carolynvs - I'm happy to help here, if you want to take the lead on the conversion. Just let me know. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
carolynvs
Mar 20, 2017
Collaborator
@tro3 I'm working on marshaling to TOML right now, using go-toml. Once I have something worth looking at, I'll start a WIP PR.
|
@tro3 I'm working on marshaling to TOML right now, using go-toml. Once I have something worth looking at, I'll start a WIP PR. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
tro3
Mar 20, 2017
Contributor
|
Okay. If there are any subtasks you want me to take on, just let me know.
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Also happy to help on sub tasks |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
rsc
Mar 28, 2017
Please just keep the files with the names and formats they already have. They are perfectly fine for the dep experiment. They will probably change in the final go command integration based on that experience, and that's fine. But don't introduce unnecessary churn by changing them before that.
rsc
commented
Mar 28, 2017
|
Please just keep the files with the names and formats they already have. They are perfectly fine for the dep experiment. They will probably change in the final go command integration based on that experience, and that's fine. But don't introduce unnecessary churn by changing them before that. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
peterbourgon
Mar 28, 2017
Member
@rsc The opportunity for comment has come and gone. We will move forward with TOML as the means to establish intermediate stability for users.
|
@rsc The opportunity for comment has come and gone. We will move forward with TOML as the means to establish intermediate stability for users. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
kris-nova
Mar 28, 2017
Contributor
We will move forward with TOML as the means to establish intermediate stability for users.
I think @peterbourgon's words here bring up a good point. Decision is made, let's get to coding.
I also agree with @rsc in the sense that iteration is always possible (if necessary). But we need to start somewhere, and it looks like we are starting with TOML.
I think @peterbourgon's words here bring up a good point. Decision is made, let's get to coding. I also agree with @rsc in the sense that iteration is always possible (if necessary). But we need to start somewhere, and it looks like we are starting with TOML. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
aajtodd
Mar 29, 2017
TOML is a good choice as is hand editing. I find this same workflow in Rust to be rather easy and intuitive. Hand edit manifest, tool generates the lock file. People can build tooling on top of it to automatically write a manifest if they really find that to be an issue.
aajtodd
commented
Mar 29, 2017
|
TOML is a good choice as is hand editing. I find this same workflow in Rust to be rather easy and intuitive. Hand edit manifest, tool generates the lock file. People can build tooling on top of it to automatically write a manifest if they really find that to be an issue. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
mitsuhiko
Mar 29, 2017
May I suggest locking this issue to contributors? I do not want to unsubscribe from the issue because I'm interested in when it is being closed but I am less than thrilled by the continued inbox spam I get from it. And I do understand the irony of me just contributing to the problem with this comment.
mitsuhiko
commented
Mar 29, 2017
|
May I suggest locking this issue to contributors? I do not want to unsubscribe from the issue because I'm interested in when it is being closed but I am less than thrilled by the continued inbox spam I get from it. And I do understand the irony of me just contributing to the problem with this comment. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
mibk
Mar 29, 2017
@mitsuhiko May I suggest https://tellmewhenitcloses.com/session/new (although I haven't tested it myself...)
mibk
commented
Mar 29, 2017
|
@mitsuhiko May I suggest https://tellmewhenitcloses.com/session/new (although I haven't tested it myself...) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
pierrre
Mar 29, 2017
May I suggest locking this issue to contributors? I do not want to unsubscribe from the issue because I'm interested in when it is being closed but I am less than thrilled by the continued inbox spam I get from it. And I do understand the irony of me just contributing to the problem with this comment.
pierrre
commented
Mar 29, 2017
|
sdboyer commentedJan 23, 2017
•
edited
Edited 1 time
-
sdboyer
edited Mar 20, 2017 (most recent)
DECISION IS MADE - we'll be using TOML.
At the time dep went public, the committee was generally in agreement that we would prefer to use TOML or YAML over JSON for the manifest and lock files. There are a few reasons for this, but the primary one is that those formats, unlike JSON, allow users to include arbitrary comments. Because the manifest is the locus of dependency decisionmaking for teams, such comments can add a lot of value:
However, in looking through the existing TOML and YAML implementations in Go, we found that none of them actually support preserving arbitrary comments when regenerating the files. dep is constantly rewriting the manifest and lock files; without support for comment preservation, each rewrite would blow away any user comments. So, without this feature, TOML and YAML lose much of their advantage over JSON.
There are other arguments for using TOML or YAML, and it is possible that the we may still elect to use them even without support for comment preservation. However, we are putting a call out - if anyone feels particularly itchy at the thought of using JSON for these files, then please, contribute comment preservation support to an existing implementation, and we'll make the switch! The deadline for this change is the relevant milestone, after which the format and structure of the manifest and lock must be stable.
Alternatively, if we've missed an implementation of TOML or YAML that does support comment preservation, or there's some other crafty way around this problem, please tell us here!
UPDATE: we've decided to generally not have the tool update the manifest, which relieves us of the need for a comment-preserving implementation.