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

Add note about clang-format versions #16

Merged
merged 2 commits into from
Jun 21, 2024
Merged

Conversation

michael-okeefe
Copy link
Member

@michael-okeefe michael-okeefe commented Mar 27, 2024

After struggling with this, just wanted to pass along this hard-earned information for others.

Description

Updates README to hopefully save others pain in setting up clang-format for their projects.

Reviewer Checklist:

  • Read the pull request description
  • Perform a code review on GitHub
  • Confirm all CI checks pass and there are no build warnings
  • Pull, build, and run automated tests locally
  • Perform manual tests of the fix or feature locally
  • Add any review comments, if applicable
  • Submit review in GitHub as either
    • Request changes, or
    • Approve
  • Iterate with author until all changes are approved
  • If you are the last reviewer to approve, merge the pull request and delete the branch

After struggling with this, just wanted to pass along this hard-earned information for others.
@michael-okeefe michael-okeefe added the documentation Improvements or additions to documentation label Mar 27, 2024
@michael-okeefe michael-okeefe requested review from nealkruis and tanaya-mankad and removed request for nealkruis and tanaya-mankad March 27, 2024 20:44
@michael-okeefe
Copy link
Member Author

OK, not sure how to add two reviewers to github... only letting me do one.

Copy link
Contributor

@tanaya-mankad tanaya-mankad left a comment

Choose a reason for hiding this comment

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

Actually, some details about clang-format versions are spelled out in detail in the guidelines.md document already, as I ran into similar issues! The directives in our clang-format file are compatible with 16. @michael-okeefe could you have a look under "Editor Settings" there and then we can consolidate our observations in the readme?

@michael-okeefe
Copy link
Member Author

Thanks, @tanaya-mankad ! Didn't actually know there was already discussion on this in the guidelines.md -- apologies for missing that.

I think the main issue was I hadn't seen guidelines and had installed clang-format 18 and couldn't figure out why things weren't working for me until I realized that even if you specify the clang-format version in your .clang-format file, it matters what version of the clang-format engine is doing the formatting and checking the formatting on the CI -- apparently since those versions were different there was slight differences in formatting behavior which caused the CI to fail. I'm not sure if that extra information needs to be added to the guidelines file or not as, had I actually followed your instructions, I wouldn't have had an issue ;-)

@tanaya-mankad
Copy link
Contributor

Let's move that critical info to the readme then! It's undoubtedly the right place for install dependencies. I'll make that change.

@michael-okeefe
Copy link
Member Author

Shall I delete this PR at this point?

Copy link
Contributor

@tanaya-mankad tanaya-mankad left a comment

Choose a reason for hiding this comment

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

If you re-spell "you're" I'll merge this. ;)

Fix spelling mistake.
@michael-okeefe
Copy link
Member Author

Done, thanks!

@tanaya-mankad tanaya-mankad merged commit fc9fe19 into main Jun 21, 2024
6 checks passed
@michael-okeefe michael-okeefe deleted the add-clang-format-note branch September 27, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants