Skip to content
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

Feature to use YAML in .butane #62

Open
jayvdb opened this issue Apr 19, 2023 · 6 comments
Open

Feature to use YAML in .butane #62

jayvdb opened this issue Apr 19, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@jayvdb
Copy link
Collaborator

jayvdb commented Apr 19, 2023

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

@jayvdb
Copy link
Collaborator Author

jayvdb commented Apr 20, 2023

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

@Electron100
Copy link
Owner

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
Copy link
Collaborator Author

jayvdb commented Apr 20, 2023

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
Copy link
Collaborator Author

jayvdb commented Apr 22, 2023

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
Copy link
Owner

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
Copy link
Collaborator Author

jayvdb commented Apr 23, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants