search

Electron100 / butane

An ORM for Rust with a focus on simplicity and on writing Rust, not SQL
Apache License 2.0
84 stars 12 forks source link

Feature to use YAML in .butane #62

Open jayvdb opened 1 year ago

jayvdb commented 1 year ago

Would you mind if I created a feature to use serde_yaml to use YAML instead of JSON for the files under .butane ?

jayvdb commented 1 year ago

Or TOML. Anything but JSON, really. ;-)

Electron100 commented 1 year ago

Hmm, talk me through the rationale. Why?

I certainly won't claim that JSON is perfect, but it's human-readable and ubiquitous. TOML is better for configuration files, but that's not really what these are. I've never been a big fan of YAML due to the indentation-dependence and general complexity, but I'll grant that it's more human-writable than JSON. But these files aren't supposed to be written by humans, they're just a persisted representation of state intended to be operated on only by the butane library/cli.

Moreover, regardless of the merits of any particular format I do think there's value in having a single format. Otherwise either everybody working on a project using the non-default format has to compile their Butane CLI with special flags when installing it, or the CLI needs to support all the formats by default and try to intelligently figure out which one any given project is using. Which is certainly doable, but it seems undesirable.

None of this is to say I'm set against it, I just want to understand the why before it goes forward.

jayvdb commented 1 year ago

The reason is JSON is terrible for being stored in a repository, especially if the file will change. The current directory especially changes with each change to our schema, and I want the diff to be readable so that checking it becomes an important part of our change review process.

jayvdb commented 1 year ago

Another example of the problem is handling merge conflicts. Line based formats handle merge conflicts better. This is most obvious in .butane/migrations/current/types.json because it doesnt use line breaks at all, but even if it does, JSON is still problematic because it cant handle "trailing commas".

Electron100 commented 1 year ago

Overall, the source control diff rationale makes sense and seems like a good motivation for a different format. I'm curious to hear more about how checking the diff of current would be an "an important part of [your] change review process" though. Current should be mechanically generated from the current Rust sources, so I'm not sure what a review would focus on? If you get any merge conflicts I'd generally recommend just re-compling to regenerate the contents of current.

jayvdb commented 1 year ago

The code review of current should verify it contains exactly and only the changes which were made to the Rust structs. Even if it will be regenerated correctly every time, the code review process will ensure everyone becomes familiar with the changes that are expected under current, and then becomes concerned if they dont see what is expected, and either learn more about butane if it was behaving correctly, or investigate if the unexpected change turns out to be a bug.