Skip to content

Conversation

@Skptak
Copy link
Contributor

@Skptak Skptak commented Aug 30, 2023

Add in a CI-CD action to clang-format files, as well as check for CRLF line endings and trailing whitespace
Add relevant test cases as well.

@Skptak Skptak force-pushed the clangFormatting branch 2 times, most recently from 09f2001 to 63ddcc2 Compare August 30, 2023 18:43
@kstribrnAmzn
Copy link
Member

Couple of things:

  1. I'd love to see smaller example files used. The example files alone for this PR are MASSIVE. In total this PR is adding roughly 60k lines. It's not exactly easy to review.
  2. It would be nice to have a comment block at the top of the example files explaining how exactly they were generated and what the problem with the file is. While the names are nice, I can't imagine that you change 50k+ lines for formatting failures

@kstribrnAmzn
Copy link
Member

Approving because I don't see any stand out problems but I obviously haven't read every single line in depth.

kstribrnAmzn
kstribrnAmzn previously approved these changes Aug 31, 2023
@Skptak
Copy link
Contributor Author

Skptak commented Aug 31, 2023

Couple of things:

1. I'd love to see smaller example files used. The example files alone for this PR are MASSIVE. In total this PR is adding roughly 60k lines. It's not exactly easy to review.

Replaced the files with short ones that are 200 lines long at most.
It was an intentional decision to select files from various FreeRTOS Repos:

  1. To show that the formatting actions then also work for files we have.
  2. To make my life easier by not needing to generate C or H files that had issues, as I just grabbed files that failed checks when implementing the clang-format action.
    Additionally, certain FreeRTOS Files will fail to parse through uncrustify using uncrustify version .72. Where the idea was to grab long ones so that if you're testing the action itself we see that these long files will handle the new formatting changes, if one was changing the .clang-format file for example.
    But not going to hold up this PR about this since both you and @paulbartell have requested shorter files.
2. It would be nice to have a comment block at the top of the example files explaining how exactly they were generated and what the problem with the file is. While the names are nice, I can't imagine that you change 50k+ lines for formatting failures

Do you still feel this way with the current naming convention?
I have also added a README explaining the purpose of these files.

…e smaller for the tests, minor changes to the action file. Add a README explaining the action and directory layout. Add a git attribute file to keep the line endings as CRLF for needed files
Soren Ptak and others added 4 commits August 31, 2023 16:35
…Also use quiet versions of the actions for use in large repos. This keeps the git log smaller so the page doesn't glitch out trying to show you the results
uint64_t minutes;
uint64_t seconds;
uint64_t msec;
} DateAndTime;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entirely a fake C file that is just used to catch formatting errors, any content in the file itself related to code is irrelevant.

#error "FCLK_FREQUENCY Error: Please enter a valid divider value"
#endif

#if( ICLK_FREQUENCY > 100000000L )
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entirely a fake C file that is just used to catch formatting errors, any content in the file itself related to code is irrelevant.

@Skptak Skptak merged commit 3e68c36 into FreeRTOS:main Sep 5, 2023
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.

4 participants