Move to TOML for manifest and lock by carolynvs · Pull Request #342 · golang/dep

Fixes #119; fixes #168.

I'm doing this in stages so that we can identify when I'm off in the weeds or need to make more decisions about the config file format.

• Vendor https://github.com/pelletier/go-toml.
• Write manifest and lock in TOML format.
• Read manifest and lock in TOML format (all tests but diff should pass)
• Remove old JSON files and logic.
• Fix output of dry-run to use TOML (diff test and all others should pass)
• (Optional) Bike-shed the look of the generated TOML, e.g. inline tables, indenting, key sorting, etc.

@carolynvs hmm...that puts me on the fence about it, as I'd generally rather have the (presumably cross-TOML implementation portable) struct tags as the canonical way of expressing this information, to minimize effort in the event that we switch TOML libs later. idk how much work that's really saving, though, so it seems like a soft requirement at worst.

Maybe some input from @pelletier here would be good - any plans to add reflection-based struct tag support to go-toml? Would you accept such a change, if offered?

Eh, that was too wishy-washy, sorry. If we can have struct tags and reflection-based parsing, great. If not, well, it seems like the one-off implementation here isn't too awful. Either way, not a reason to derail the current track.

Is there are reason you're keeping the JSON struct tags around?

@sdboyer yes, reflection-based (un)marshaling is the next thing that I'd like to implement in go-toml. no eta on that though, but pull requests are welcome :)

@pelletier - I have some time coming up the next few weeks. Mind if I take a crack at the reflection-based marshaling? If so, I'll open an issue at go-toml and get you a WIP PR in the short term to make sure you approve of the structure before I dive in too deep. Thoughts?

@sdboyer No reason, just hadn't gotten around to removing all the json stuff yet. 😃 I've added a task to the PR to make that more clear.

• Changed manifest format. dependencies and overrides use an array of tables instead of maps now, which is easier to work with.

  ignores = ["github.com/foo/bar"]
  
  [[dependencies]]
    name = "github.com/sdboyer/gps"
    version = ">=0.12.0, <1.0.0"
  
  [[dependencies]]
    name = "github.com/babble/brook"
    revision = "d05d5aca9f895d19e9265839bffeadd74a2d2ecb"
  
  [[overrides]]
    branch = "master"
    name = "github.com/sdboyer/gps"
    source = "https://github.com/sdboyer/gps"

• Update all the test files from JSON to TOML

• Poor man's mapping from TOML to structs. Can't wait for Reflection-based marshaling / unmarshaling pelletier/go-toml#149! ❤️

Next up I'm fixing the lock diff format and looking sorting bug (tests pass then fail due to sorting).

Couldn't now overrides be just an attribute on the dependency itself?

@omeid Let's not make changes to the config file structure in this PR, beyond the initial scope of switching from JSON to TOML. 😀 It's already a pretty big change, and I want to limit the amount of regressions/confusion it may cause.

For generated TOML, I think it would be nice to use inline tables to get each dependency onto a single line.

I'm think that may work better for diffs/merges, though I'm not totally sure. Also it's nice for rearranging the dependencies by movings lines up/down in an editor (for super organized types).

I can imagine counter arguments though, as lines could get pretty long when there are a number of options specified.

@nathany I think that depends on how we feel about long lines in the file. Personally, I find it hard to read.

For example, here's what I think the lock would look like with inline tables. Note, I am cheating a bit by using "." for the package list, to make the shortest possible entry.

memo = ""
projects = [
 { name = "github.com/sdboyer/deptest", packages = ["."], revision = "ff2948a2ac8f538c4ecd55962e919d1e13e74baf", version = "v0.8.0" }
 { name = "github.com/sdboyer/deptestdos", packages = ["."], revision = "a0196baa11ea047dd65037287451d36b861b00ea", version = "1.0.0" }
]

Though I'm not 100% sure that's how an array of inline tables should look? Either way support for changing the formatter in go-toml would be needed, not sure about the parser.

Okay. Those are some pretty long lines.

It's probably best to just use it for a while to see how well diffs/merges work with the multi-line syntax. Thanks

I've updated the lock diff to output matching TOML. I've also tried out @tro3 's reflection based mapping, it's super close and I think it's worth waiting for it, removes a TON of code. 😃

We need to roll the file name change into this, too, so that we minimize the amount of change that goes in at once. We're still pondering those, but...

Cursory look has me thinking this is pretty good. I'll look more soon, I hope later tonight :)

Mostly nits, a couple slightly bigger things.

Sorry for the slow action on this!

@sdboyer @tro3 Right now go-toml doesn't preserve the ordering of the fields when marshaling, instead simply outputting them in alphabetical order:

[[projects]]
  branch = "master"
  name = "github.com/pelletier/go-buffruneio"
  packages = ["."]
  revision = "c37440a7cf42ac63b919c752ca73a85067e05992"

instead of this (which matches the ordering of the fields as defined on the struct which is what encoding/json does)

[[projects]]
  name = "github.com/pelletier/go-buffruneio"
  branch = "master"
  revision = "c37440a7cf42ac63b919c752ca73a85067e05992"
  packages = ["."]

Is this something that I need to address as part of this PR, in the name of not changing the file structure again later? It may be a big change to how the TomlTree is represented internally and could be a lot of work, so I wanted to get your thoughts (or maybe @tro3 you see a clean way of supporting that)?

• Rebased, resolving the merge conflicts from updating gps
• Added constants for the file names to the lock package
• Updated LockDiff formatting to the toml struct tags

Looking into making the auto-indentation configurable next.

Is it really needed? Don't know if he key order is important. In any case, I think it's an easy tweak to go-toml, as I recall an explicit sorting of the keys. If correct, this would just have to be bypassed with a configure option of some sort. I'm on vacation though, so if it needs to be me, it will have to wait a while.

I spoke with @sdboyer and we will leave off trying to make the toml file format "pretty", so I am not going to try to tackle the indenting or sorting in this PR. I believe I've hit all the rest of the review feedback so it's ready for another look.

OK for filenames, let's do Gopkg.toml and Gopkg.lock.

Is it possible, or was it already decided not, to read the old format and update the relevant files?

As the migration path, as I understand it, would be to remove the old files and generate new lock files - which would lock to different revisions?

Detecting existing files, reading them and rewriting to new files and new format would obviously be ideal from an early user's perspective. Perhaps a flag, or a separate application (depfix)?

I don't see this as a major requirement at all, just asking.

Sorry for delay on merge, here. I was trying to get things together with a blog for dep (re: #294), and I put this in the queue behind doing that. Looks like I should have that blog done tonight, and I plan to merge this once it's all set up 😄

And in, finally. Thanks! 🎉 🎉 🎉 🎉