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

clarify style guidelines in CONTRIBUTING.md / set up clang-format #150

Open
superwhiskers opened this issue Jun 3, 2024 · 3 comments
Open
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request question Further information is requested

Comments

@superwhiskers
Copy link
Collaborator

related to #54 / #82, albeit it deserves a dedicated pull request to unbundle it from the addition of pre-commit

rationale

currently, CONTRIBUTING.md is insufficiently precise in some ways and the repository's existing .clang-format file is not used and does not match the conventions within CONTRIBUTING.md exactly. clarifying the style guidelines and setting up .clang-format would help maintain a consistent style across the codebase and potentially make it more welcoming to newcomers

considerations

  • how is existing code formatted in relation to what is documented in CONTRIBUTING.md?
  • how should the codified style be imposed upon existing code?
    • should we incrementally reformat code when it is touched, or should we reformat it all in one go?
  • should we add a precommit hook?
  • what additional linting tools should we use and how/when should they be applied?

initial suggestions

  • drop Add pre-commit configuration + GitHub Actions #54 & Precommit addition to the repo #82 for now
  • open a new pull request deleting the existing .clang-format file in .github/workflows1 since it isn't used and remove the dead code within the root CMakeLists.txt2 that references it
  • create a new .clang-format at the repository's root which takes into account existing style conventions + the stuff within CONTRIBUTING.md, with a priority toward the latter, updating CONTRIBUTING.md accordingly
  • do not apply the styling to existing code until a solution is agreed upon

Footnotes

  1. https://github.com/ORNL/ReSolve/blob/develop/.github/workflows/.clang-format

  2. https://github.com/ORNL/ReSolve/blob/d913854aed438af040f6ecd5bee052da3a861baa/CMakeLists.txt#L56-L58

@superwhiskers superwhiskers added the enhancement New feature or request label Jun 3, 2024
@superwhiskers superwhiskers self-assigned this Jun 3, 2024
superwhiskers added a commit that referenced this issue Jun 3, 2024
- drop dead code within `CMakeLists.txt`
- remove old `.github/workflows/.clang-format`
- create new `.clang-format` at the repository root, with settings based on observed patterns & `CONTRIBUTING.md`
@superwhiskers superwhiskers added documentation Improvements or additions to documentation question Further information is requested labels Jun 3, 2024
@superwhiskers superwhiskers linked a pull request Jun 3, 2024 that will close this issue
@superwhiskers superwhiskers removed a link to a pull request Jun 3, 2024
@superwhiskers
Copy link
Collaborator Author

some additional comments:

@pelesh
Copy link
Collaborator

pelesh commented Jun 4, 2024

@cameronrutherford and @ryandanehy, please chime in.

CC: @kswirydo

@cameronrutherford
Copy link
Collaborator

Closed #82. I think we could close #54 without doing C++ formatting, and then have the clang-format and clang-tidy hooks added in another PR/Issue.

+1 to the clang-tidy suggestion. Might be worth discussion in another issue and closing out one thing at a time.

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 enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants