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

Apply a formatter for CMake files #5973

Merged
merged 10 commits into from
Dec 1, 2023
Merged

Apply a formatter for CMake files #5973

merged 10 commits into from
Dec 1, 2023

Conversation

Gold856
Copy link
Contributor

@Gold856 Gold856 commented Nov 29, 2023

Formats all CMakeLists with gersemi.

@Gold856 Gold856 requested a review from a team as a code owner November 29, 2023 03:55
@calcmogul
Copy link
Member

calcmogul commented Nov 29, 2023

Should we add gersemi to wpiformat? We already use black for Python formatting (pyformat.py), which has similar cmdline arguments.

@Gold856
Copy link
Contributor Author

Gold856 commented Nov 29, 2023

I think that's a good idea. I'll make a PR over there and change this one accordingly.

@calcmogul
Copy link
Member

Should we generate a default .gersemirc file? What kinds of things does it let us configure? Are we OK with the default formatting @PeterJohnson?

@calcmogul
Copy link
Member

wpiformat PR is wpilibsuite/styleguide#265.

@Gold856
Copy link
Contributor Author

Gold856 commented Nov 29, 2023

A .gersemirc file allows you to configure the same things the CLI allows you to configure. I'm going to add one because that allows us to change the settings without having to change it in wpiformat, which means we don't need to commit to wpiformat to make a formatting change, then commit to allwpilib to format it.

@Gold856 Gold856 marked this pull request as draft November 29, 2023 04:47
@Gold856
Copy link
Contributor Author

Gold856 commented Nov 29, 2023

/format

@Gold856 Gold856 changed the title Add a formatter for CMake files Apply a formatter for CMake files Nov 30, 2023
@Gold856 Gold856 marked this pull request as ready for review November 30, 2023 02:56
@Gold856 Gold856 requested review from PeterJohnson and a team as code owners November 30, 2023 02:56
@PeterJohnson
Copy link
Member

We should exclude cmake/toolchains--these files are mostly copied from OpenCV.
I personally think we should hold to 80, or 100 at most. 150 is way too long to be readable, particularly when it appears the tool combines lines that were previously manually split.

@calcmogul
Copy link
Member

This should make it skip the cmake/toolchains folder:

diff --git a/.styleguide b/.styleguide
index 1ba4e1dcc..f67c100e0 100644
--- a/.styleguide
+++ b/.styleguide
@@ -10,6 +10,7 @@ cppSrcFileInclude {
 }
 
 modifiableFileExclude {
+  cmake/toolchains/
   \.patch$
   gradlew
 }

@calcmogul
Copy link
Member

Making this change will make wpiformat run on CMake files (you'll need to merge in main):

diff --git a/.github/workflows/lint-format.yml b/.github/workflows/lint-format.yml
index cac0a2f8a..6545600a9 100644
--- a/.github/workflows/lint-format.yml
+++ b/.github/workflows/lint-format.yml
@@ -27,7 +27,7 @@ jobs:
         with:
           python-version: 3.8
       - name: Install wpiformat
-        run: pip3 install wpiformat==2023.34
+        run: pip3 install wpiformat==2023.35
       - name: Run
         run: wpiformat
       - name: Check output

.gersemirc Outdated Show resolved Hide resolved
@Gold856
Copy link
Contributor Author

Gold856 commented Nov 30, 2023

I don't know why, but wpiformat now wipes out all CMake files.

@PeterJohnson
Copy link
Member

Tyler just merged wpilibsuite/styleguide#265, so that could be why?

@Gold856
Copy link
Contributor Author

Gold856 commented Nov 30, 2023

Well, yeah. I know it's my PR. What I'm trying to figure out is why it's wiping the files.

@calcmogul
Copy link
Member

The task config in wpiformat 2023.35 was bad. Upgrade to 2023.36.

@Gold856
Copy link
Contributor Author

Gold856 commented Nov 30, 2023

/format

@PeterJohnson PeterJohnson merged commit 4fcf0b2 into wpilibsuite:main Dec 1, 2023
24 checks passed
Starlight220 pushed a commit to Starlight220/allwpilib that referenced this pull request Dec 1, 2023
@Gold856 Gold856 deleted the format-cmake branch December 1, 2023 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants