sleiner / pizmidi

A collection of helpful tools when working with MIDI
GNU General Public License v3.0
23 stars 4 forks source link

clang-format: fix schema validation and update definitions #22

Closed loowps closed 2 years ago

loowps commented 2 years ago

According to CLion some values aren't inline with the schema validation.

sleiner commented 2 years ago

Also, this PR made me aware of the fact that the clang-format CI job was broken, so I fixed it 👍🏻 Thanks! :-)

loowps commented 2 years ago

I reverted and committed the changes separately. But I'm a little bit lost here as I'm used to squash merging to main on PR complete. Seems like the quality gate is evaluated for all commits and not the final diff..

loowps commented 2 years ago

Thanks for the explanation. I'm more a visual guy and use sourcetree, but now it seems to be more clear to me (had to enable force push). Quite a nice way to rewrite the git history on a feature branch. Thx!

Sadly it seems like the current main doesn't comply with the formatting and therefore the first commit already breaks the check?

sleiner commented 2 years ago

I'm more a visual guy and use sourcetree

Me to actually (although I'm using Fork instead of SourceTree), but these steps are much easier to explain with the command line 😅

Sadly it seems like the current main doesn't comply with the formatting

The formatting on main is correct... at least as far as clang-format 14 is concerned. The formatting changes from aa2bcd6f0da0d38a8c17a26182595367d6a8f3fc are only preset if you use clang-format 15. The solution for that is separating the clang-format update into a separate commit. Since that requires some changes to CI scripts (git-clang-format has changed its return values) and I also noticed another problem when stacking multiple changes to .clang-format within a single pull request, I have separated this into a separate pull request: #25. Once that gets merged, please rebase your branch unto main :-)

loowps commented 2 years ago

Me to actually (although I'm using Fork instead of SourceTree), but these steps are much easier to explain with the command line 😅

Ah, nice! IIRC I used Fork for some time with the idea to replace ST and liked it even more! But when I got used the differences, they unexpectedly went commercial. So I ditched it again.

Explanation was really helpful!

The formatting on main is correct... at least as far as clang-format 14 is concerned. The formatting changes from aa2bcd6 are only preset if you use clang-format 15. The solution for that is separating the clang-format update into a separate commit. Since that requires some changes to CI scripts (git-clang-format has changed its return values) and I also noticed another problem when stacking multiple changes to .clang-format within a single pull request, I have separated this into a separate pull request: #25. Once that gets merged, please rebase your branch unto main :-)

Thanks, good work! Was a little bit naive on my end to simply increment the version without looking for breaking changes, sry! I rebased the branch accordinly.

sleiner commented 2 years ago

So, one more nitpick about the commit log 🙈 Currently, the first line of the message is really long for many commits. This leads to weirdness in a multitude of tools displaying the log. See GitHub as an example:

image

Could you please rephrase those to a short summary on the first line (<= 50 characters) and paragraphs with around 72 characters per line afterwards (see this writeup for reference)?

Example:

clang-format: Set InsertBraces to true

This wraps single line control statements that have no braces yet, so
all control statements are equal and can be easily extended by a second
line.
loowps commented 2 years ago

So, one more nitpick about the commit log 🙈 Currently, the first line of the message is really long for many commits. This leads to weirdness in a multitude of tools displaying the log.

Np, I fully understand your request. With cherry picking and ammending the commit message it can be adapted without much effort.. but tbh overall feels kind of outdated to be restricted to a certain amount of chars even if the credo is the shorter the better