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 formatter #2004

Closed
adamjstewart opened this issue Apr 17, 2024 · 16 comments · Fixed by #2018
Closed

Add YAML formatter #2004

adamjstewart opened this issue Apr 17, 2024 · 16 comments · Fixed by #2018
Labels
good first issue A good issue for a new contributor to work on testing Continuous integration testing

Comments

@adamjstewart
Copy link
Collaborator

Summary

We should add a YAML formatter to CI

Rationale

Indentation is inconsistent, and I frequently have to remind people to add EOL chars before EOF chars.

Implementation

Bunch of options:

Alternatives

No response

Additional information

No response

@adamjstewart adamjstewart added testing Continuous integration testing good first issue A good issue for a new contributor to work on labels Apr 17, 2024
@Domejko
Copy link
Contributor

Domejko commented Apr 17, 2024

Hey @adamjstewart

I can take care of this.

I assume that formatting should be run on all files, not just some specific folder ? Also to clarify, this job should be implemented in style.yaml CI ?

@adamjstewart
Copy link
Collaborator Author

Yes, all *.yaml and *.yml files.

Yes, .github/workflows/style.yaml.

It would be preferable to use something available on PyPI so we can explicitly list it as a dependency and install it with pip. Maybe you can review the options and propose which tool is best and why before actually making any code changes?

@Domejko
Copy link
Contributor

Domejko commented Apr 17, 2024

Yes that will be most convenient. I will review then available packages and will let you know which ones I would suggest.

@Domejko
Copy link
Contributor

Domejko commented Apr 17, 2024

From what I have searched Google (unofficial) yamlfmt if well recommended but it's written in Go and it's not in PyPI, it is similar with Prettier that can cover not only YAML but many other formats as well but it needs different package managers like NPM, Yarn etc.

In this case I would decide on yamlfix. It's available on PyPI, have good and clear documentation can be simply configured in pyproject.toml if additional options are needed. Is maintained and updated since few years so it doesn't look like dead project.

@adamjstewart
Copy link
Collaborator Author

yamlfmt's dependency on go and prettier's dependency on npm are unideal.

yamlfix appears to be somewhat dead: lyz-code/yamlfix#272

What about pretty-yaml? Are there any other alternatives we can consider?

@Domejko
Copy link
Contributor

Domejko commented Apr 18, 2024

True, sorry I haven't noticed that.

Pretty-yaml is very simple and seams to not support going thru all the directories in search for *.yaml, *.yml files. On top of that it's replacing true with yes and false with no so it doesn't suet our needs.

I will be keep on searching and will let you know as soon as I find something.

@adamjstewart
Copy link
Collaborator Author

adamjstewart commented Apr 18, 2024

In YAML, true == yes: https://yaml.org/type/bool.html

I personally prefer true, but I do want to ensure that all Booleans follow the same pattern. I'm surprised pretty-yaml didn't follow the canonical definition here.

@Domejko
Copy link
Contributor

Domejko commented Apr 18, 2024

Yes I noticed that here in code is used true and false so this along with inability to use it recursively disqualifies pretty-yaml as a potential package to use.

I did search since last comment and I could not find anything that could be used. Packages either do not support CLI, or are dead projects that haven't been touched for at lest a year or more.
Prettier is dominating in this field.

Then I think in this case we could use yamlfix even tho at the moment maintainer since 5 months is too busy, he says that eventually will comeback to project. It supports going through files recursively, can be easily configured, have quite few options and well written documentation. I can test if it wont throw any unsolved errors while running on torchgeo code.

There is also yamllint but this is only a linter, it would only prompt user where the errors are but it wont fix them on it self.

@adamjstewart
Copy link
Collaborator Author

Let me think about this. It might not be horrible to use a go/npm package since it's only used by developers, not users. I'm also curious how many files would be changed if we used something like prettier. We do need something for JSON files too for .ipynb files...

@Domejko
Copy link
Contributor

Domejko commented Apr 19, 2024

When it's a part of CI it don't make much of a difference to us, it's just matter of using actions/setup-node@v4 or actions/setup-go@v5 instead of actions/setup-python@v5. On the other hand Prettier could be also used as pre-commit hook but then of course node would had to be installed with Prettier as dependency.

It's possible that initially it could slightly change all the files since it's very opinionated, as it's devs say "Prettier is not a kitchen-sink code formatter that attempts to print your code in any way you wish. It is opinionated.". But still it have some options that can be always adjusted. If JSON files also would had to be handled then Prettier is a best choice.

@adamjstewart
Copy link
Collaborator Author

I'm fine with opinionated, that's the whole point of a formatter/linter.

@Domejko
Copy link
Contributor

Domejko commented Apr 22, 2024

What is the final decision on that matter ? Will we use Prettier ?

@adamjstewart
Copy link
Collaborator Author

I think prettier is probably best. Can you open a PR that uses prettier to see what the changes look like?

@Domejko
Copy link
Contributor

Domejko commented Apr 22, 2024

This is how it looks like while it was run without any additional options with just npx prettier . --write

@Domejko
Copy link
Contributor

Domejko commented Apr 25, 2024

Have you seen changes in PR #2018 ?

@adamjstewart
Copy link
Collaborator Author

Seen yes, but haven't had time to review in detail yet. Apologies for the wait...

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue A good issue for a new contributor to work on testing Continuous integration testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants