-
Notifications
You must be signed in to change notification settings - Fork 31
Add pre-commit to sort yml files #249
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
base: main
Are you sure you want to change the base?
Conversation
082cd23
to
cd47e68
Compare
the amount of yaml packages for python... 🤦 but good idea I'll have a look tomorrow! |
ah agree, this package has nice feature by preserving the comments from the file :), if you think this package adds more burden we can go with defaul yml parser and we have to think about preserving the comments if there are any exists :). |
Rather than perform a sort, why not reject commits that are not already sorted? |
There are multiple reasons - most of them about lowering friction, saving time, energy, focus and cognitive effort of both maintainers and contributors - and allowing them to focus on what really matters. Because when you run Of course it does not apply to those who do not install pre-commit. But it's mainly targetted to those who do (and this way they actually improve lives of maintainers by saving their energy and focus on things that can be done by the contributor on their own, way before attention of the maintainer is dragged to the change). Also it's a way to save contributor's time. Eventually they do not have to learn how to format code, how to sort it, etc - if all that can be fully automated, it's a useles knowledge and a lot of time saved when those easy to automate things can be done without conscious effort of the contributor. That also requires some education - but pre-commit actually has it also built-in, when the pre-commit fails in CI it actually asks you to install it locally - it provides helpful instructions on what to do. |
cd47e68
to
10921b8
Compare
I'll just point out that it's not possible in general to determine where header comment stops and comments for the first entry start. Similarly, if a block has comments, these may be placed before and/or after the block. Again, it's not possible in general to determine to which block such comments belong. |
Yep. In those cases code review should point it out and things could be fixed afterwards. Likely that's a very low percentage number of cases, but all detectable and fixable during code reviews (which are anyhow mandatory) "Done is better than perfect". |
yeah comments are bit hard to track , i see ruyaml.yml handling it nicely it’s keeping comments after sorting it in right place. |
Maybe add an Action which checks whether the actions are properly sorted. Without this, it's possible that the next change from someone who does use the check will show both their change and the sorting fix, making it harder to see what the actual change is. |
+10. I thought it's already there :) This is the power of pre-commit - usually what you should do in PR is to run:
Then, when it fails it shows what it "would have done" and tell you what to do. |
83794d3
to
884e7b2
Compare
884e7b2
to
306552e
Compare
Added ci workflow to run pre-commit. |
Co-authored-by: Jarek Potiuk <[email protected]> Signed-off-by: GPK <[email protected]>
Co-authored-by: Jarek Potiuk <[email protected]> Signed-off-by: GPK <[email protected]>
@potiuk thanks nice suggestion :) |
Adding yml file sorting to sort actions.yml.
First i am getting pre-commit hook to sort actions file.
Next steps will open new PRs to update gateway script, i think currently gateway script uses
ruyaml
, i feel it would be good if we can use ruamel.yaml, in place ofruyaml
. looks like ruamel.yaml is actively maintaining and it has good featurescloses #203
Request for adding a new GitHub Action to the allow list
Overview
Name of action:
URL of action:
Version to pin to (hash only):
Permissions
Related Actions
Checklist
You should be able to check most of these boxes for an action to be considered for review.
Please check all boxes that currently apply: