Add CMake format #5
Conversation
| ${ICEBERG_PUFFIN_SOURCES} | ||
| PRIVATE_INCLUDES | ||
| ${ICEBERG_PUFFIN_INCLUDES}) | ||
| add_iceberg_lib(iceberg_puffin |
There was a problem hiding this comment.
Might be personal preference but I find the original formatting more readable. I don't like this much indentation. Do you think there is a way to configure that?
There was a problem hiding this comment.
I simply used the cmake-format plugin in vscode with default setting which formatted these CMake files.
There was a problem hiding this comment.
I simply used the
cmake-formatplugin in vscode with default setting which formatted these CMake files.
Yeah, but we need to check the format when PRs are created, I did not include the CI part because I not sure what is the best way to do it. We might consider that together with clang-format.
There was a problem hiding this comment.
Before adding source files, I think it would be good to conclude this PR. Any pending concerns? If not, please approve the PR 👍
|
One general question: I see the command in the description that leverages this change to format CMake files. It's up to the contributor to run this manually, and I'm wondering if it makes sense to introduce some build script that runs this, the clang format, and also compiles the project. We'd have more guarantee then that these format requirements are met |
Make sense, I will try to find some gh actions that are fit for these tasks, maybe after #7 merged. |
There was a problem hiding this comment.
As discussed, some follow-up PRs would be great for running the formatting mechanisms.
use the following command to apply the rules to all existing CMakeLists.txt find . -name CMakeLists.txt | xargs cmake-format -c cmake-format.py -i Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
caa9888 to
d5e9a93
Compare
There was a problem hiding this comment.
+1
nit: should we put cmake-format.py into a sub-folder like scripts or something?
It's not a script, it's a config file for the |
We should just use pre-commit, which is able to do both. |
|
Thanks for raising this PR @zhjwpku and thanks for the review @gaborkaszab @pitrou and @wgtmac
I love pre-commit 👍 |
use the following command to apply the rules to all existing CMakeLists.txt
find . -name CMakeLists.txt | xargs cmake-format -c cmake-format.py -i