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

Adding yamlfix formatter #143

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leahwicz
Copy link
Contributor

resolves #137

Problem

Have a linter and formatter for GitHub workflows

Solution

This is an alternative formatter to the one proposed here: #141

@cla-bot cla-bot bot added the cla:yes label Aug 30, 2024
@leahwicz
Copy link
Contributor Author

@mikealfare so I dug a little more after the comments you left on draft PR #141 and I think I found a better solution. You mentioned long/short list form which my original proposal did not have an option to enforce. I ended up finding yamlfix as an alternative where we can be flexible with the configuration and it also will autofix things for you. If you like this option instead, I can pursue implementing this one instead with whatever configurations that we would want to enforce.

@@ -0,0 +1,5 @@
explicit_start=false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikealfare we can configure what we want here. The default currently enforces a character limit per line which is what a lot of these changes are. Before I would merge any of these, I would want to test each workflow to make sure the line break doesn't mess anything up. I would also argue that we should format quotes to all be the same but that would also require testing the workflows as well to make sure that doesn't change behavior.

Copy link
Contributor

@mikealfare mikealfare left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestion. I only have two concerns really.

First, it would be nice if yamlfix supported command line args so that we could avoid the separate config file. It's nice having all of this config in one place in .pre-commit-config.yaml.

Second, I need to see if we can use this tool since it's GNU GPLv3. This license combines with the Apache License, but the result needs to be licensed under GNU GPLv3. We're not actually shipping it as part of the product, but it's contributing to the quality of it. I'll consult internally to see.

@leahwicz
Copy link
Contributor Author

First, it would be nice if yamlfix supported command line args so that we could avoid the separate config file. It's nice having all of this config in one place in .pre-commit-config.yaml.

@mikealfare we can't pass the configuration in as args unfortunately (I know it seems odd). The other alternative is placing them in the pyproject.toml that already exists so we could consolidate to that file as an option.

Second, I need to see if we can use this tool since it's GNU GPLv3. This license combines with the Apache License, but the result needs to be licensed under GNU GPLv3. We're not actually shipping it as part of the product, but it's contributing to the quality of it. I'll consult internally to see.

Also, good call on the license. This was just an idea so we can go back to the original proposal #141 if that is better. Just let me know what makes sense to everyone. I'm happy to do either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI/CD Improvements] Standardize and automate formatting for workflows
2 participants