-
Notifications
You must be signed in to change notification settings - Fork 47
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
Switch from cmake-format to gersemi for formatting CMake listfiles #722
Conversation
Waiting on #720 to be merged. |
434b669
to
774a97e
Compare
.gersemirc
Outdated
line_length: 80 | ||
cache: true | ||
indent: 2 | ||
quiet: true | ||
warn_about_unknown_commands: false | ||
workers: 8 | ||
list_expansion: favour-inlining |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary ?
Is it going to be necessary on each and every project of ours ?
Couldn't we just use some default settings instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to put the options that would create the least changes. I think some of these might be the default values though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care about the number of changes ?
Those are already 3k on a 15k codebase, so IMHO it could be 2 or 3 times that and we would not see the difference, as long as it is in a single commit documented in .git-blame-ignore-revs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, in that case, I remove some of the options in the gersemirc file.
42cd359
to
610976f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure about that indent: 2
, but I'm not going to block this PR for that either.
Before merging, I think we should clean git history, with:
- update .pre-commit-config.yml + setup .gersemirc
- pre-commit run -a
- add previous to .git-blame-ignore-revs
The default indent in gersemi is 4. |
Remove .cmake-format.py
043c3c6
to
c9fa8c8
Compare
Done @nim65s 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be sure to replace all # cmake-format: by # gersemi:
Also, I'm curious on how well gersemi handle custom command: https://github.com/BlankSpruce/gersemi?tab=readme-ov-file#lets-make-a-deal
On Pinocchio with cmake-format, I had to do this: https://github.com/stack-of-tasks/pinocchio/blob/devel/.cmake-format.yaml
Last, did you know if gersemi is handling comment in a better way ? cmake-format was joining consecutive line together, make huge comment block.
We can do the custom command stuff later. Checking for this would require some more in-depth look at our own code so that gersemi can pick up definitions, or introduce stubs to tell gersemi how args should be formatted. |
c9fa8c8
to
a579299
Compare
a579299
to
f743a57
Compare
a5267d9
to
ad86c18
Compare
12c9e0b
to
f9111e5
Compare
This is a rather large PR (due to reformatting many, many files)
Closes #719