Skip to content

Conversation

@ffoulkes
Copy link
Contributor

No description provided.

Signed-off-by: Derek G Foster <derek.foster@intel.com>
@ffoulkes ffoulkes requested review from 5abeel and aashishkuma June 12, 2023 13:30
@ffoulkes ffoulkes requested a review from nupuruttarwar as a code owner June 12, 2023 13:30
@5abeel
Copy link
Collaborator

5abeel commented Jun 12, 2023

Does this require clang-format installed in dev environment?

@ffoulkes
Copy link
Contributor Author

ffoulkes commented Jun 12, 2023

Does this require clang-format installed in dev environment?

No, it doesn't. It specifies the defaults to use if you run clang-format without explicit options.

I've begun running clang-format on newly developed files (unit tests). We'll need to choose the right time to begin reformatting existing files. (Probably not before 23.07, especially if it makes wrenching changes to a file.)

This particular file was copied from stratum.

Copy link
Collaborator

@5abeel 5abeel left a comment

Choose a reason for hiding this comment

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

LGTM

@nupuruttarwar
Copy link
Contributor

@ffoulkes I remember our conversation about clang format as a recommended for stratum codebase. Should we make it as part of some guideline?

Copy link
Contributor

@nupuruttarwar nupuruttarwar left a comment

Choose a reason for hiding this comment

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

LGTM

@5abeel
Copy link
Collaborator

5abeel commented Jun 12, 2023

@ffoulkes I remember our conversation about clang format as a recommended for stratum codebase. Should we make it as part of some guideline?

Might be good to do that (if you have the time, based on priority list)

@nupuruttarwar
Copy link
Contributor

@ffoulkes I remember our conversation about clang format as a recommended for stratum codebase. Should we make it as part of some guideline?

Might be good to do that (if you have the time, based on priority list)

Sure, we can take this as part of documentation update.

@ffoulkes
Copy link
Contributor Author

ffoulkes commented Jun 12, 2023

@ffoulkes I remember our conversation about clang format as a recommended for stratum codebase. Should we make it as part of some guideline?

Yes, we should.

As I see it, the process would be something along the lines of:

  • Develop a guideline
  • Commit .clang-format files where appropriate
  • Develop a GitHub action to run clang-format in check mode
  • Pick an appropriate moment to put the policy into effect
  • Let people know that it's going into effect
  • Reformat and push updates in batches
  • Enable the GitHub checker
  • Advise developers to
    • Update their working repositories
    • Run clang-format before pushing updates

Basically, we want reformatting changes to be separate from content changes.

A developer can:

  • Run clang-format on their work in progress
  • Stash their work
  • Pull a fresh copy of the repository
  • Pop or apply the stashed copy
  • Deal with merge errors and commit

If you want to take out some insurance, you can

  • git stash your WIP
  • git stash apply to get your WIP back without removing the stashed copy
  • clang-format for reformat your WIP
  • git stash again, so save your reformatted WIP
  • pull fresh copies of the repos
  • git stash apply (or git stash pop) to merge your WIP with the repo code
  • Deal with merge errors and commit
  • Push and then git stash drop to clean up

The stash on top of the stack should merge fairly cleanly.
If something goes wrong, you've still both formatted and unformatted copies of your WIP to fall back on.

@ffoulkes ffoulkes merged commit d00cad3 into main Jun 12, 2023
@ffoulkes ffoulkes deleted the clang-format branch June 12, 2023 18:18
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