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

Add YAML middleware that behaves like TOML and JSON #913

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Dekkonot
Copy link
Member

After adding TOML in addition to JSON, it seems reasonable to also support YAML doing the same thing. However, I hate yaml, so I'm not sure whether we should have it be a default middleware.

I will leave it up to the committee (anyone who reads this) to weigh in on whether we should just by-default sync .yml files or if we should just expose a middleware for them.

@Dekkonot Dekkonot added type: enhancement Feature or improvement that should potentially happen scope: cli Relevant to the Rojo CLI labels May 13, 2024
@boatbomber
Copy link
Member

I think that YAML is common enough that it should be supported out of the box, personal tastes aside. I don't think anyone likes YAML, but everyone uses it anyway.

Copy link
Member

@Kampfkarren Kampfkarren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that yaml might make sense to just have on by default since it's obvious what the intent is if you put a .yml file in the codebase. No real passionate opinion there but I sway on default

Also are the docs stored in this repo? Should we update them?

src/snapshot_middleware/yaml.rs Outdated Show resolved Hide resolved
src/snapshot_middleware/yaml.rs Outdated Show resolved Hide resolved
@UpliftMacaw
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: cli Relevant to the Rojo CLI type: enhancement Feature or improvement that should potentially happen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants