KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
834 stars 222 forks source link

Add clang-format support #913

Closed MathiasMagnus closed 6 days ago

MathiasMagnus commented 1 month ago

This work if the continuation of #889.

Notes for the reviewers

Some comments from the original work addressed here, most notably:

Aside from explicit comments:

Should other formatting changes need refining, discussions may continue here.

MathiasMagnus commented 1 month ago

I think this work is complete. The current CI failure doesn't seem to be related to the changes made here.

MarkCallow commented 3 weeks ago

You need to rebase or merge main again to pick up the workaround for the build issue on GitHub Actions. The latest runner image is only partially fixed.

MarkCallow commented 1 week ago

@MathiasMagnus please address the outstanding issues and complete this work.

MathiasMagnus commented 1 week ago

@MarkCallow We created this PR out of courtesy, because this task did not fit in the original project timeline, so we are unable to address all the specific personal styling and tooling requests. I added documentation on the minimal requirement to reduce CI friction and noted how to configure VS Code to not trip CI. Vim is too deep a rabbit a hole and I don't have the time to give anything I document in XCode a test drive.

As for the rest of the outstanding formatting related inquiries: instead of whack-a-mole-ing the formatting issues one-by-one and aligning them to the desired format, I reverted the "Initial formatting" commit and just commit the machinery enabling the enforced formatting.

Because clang-format can't run on an entire source tree, all the tools drive it file-by-file. Should you want to tweak the .clang-format files to perfection, I used the following one-liner on Windows:

# list folders excluding build and .venv | recurse into all | filter files matching regex | for each of the matches { run clang-format inplace using style files and full path to file }
gci -exc build,.venv | gci -rec | where -prop Name -match '^.*\.((((c|C)(c|pp|xx|\+\+)?$)|((h|H)h?(pp|xx|\+\+)?$))|(ino|pde|proto|cu))$' | % { clang-format.exe -i --style=file $_.FullName }

I leave implementing similar logic in the shell of their choice to the end-user. (The regex I borrowed from the script run by the CI action.)

In summary, all the infrastructure for clang-format is there, please tweak it for your needs as you see fit.

MarkCallow commented 1 week ago

I am not happy ...

We created this PR out of courtesy, because this task did not fit in the original project timeline, so we are unable to address all the specific personal styling and tooling requests.

I thought providing a PR with initial styling was part of the contract. How else would you deliver the project work, if not by a PR?

Instead of whack-a-mole-ing the formatting issues one-by-one and aligning them to the desired format, I reverted the "Initial formatting" commit and just commit the machinery enabling the enforced formatting.

There is no whack-a-mole on formatting issues here. The related open conversations are everything. I count 2 format change requests and requests to exclude 4 files from formatting. That is not a huge amount of work.

Please restore the formatting making the small number of requested changes.

I understand that documenting this for vim and xcode is potentially time-consuming, if you don't already have the knowledge. Omitting that part of my request is okay.

aqnuep commented 1 week ago

I thought providing a PR with initial styling was part of the contract. How else would you deliver the project work, if not by a PR?

Yes, but the contract ended at the end of March with the understanding that due to the additional requests we ran out of time and budget to do the clang-format change. We simply offered to put together a PR anyway afterwards, out of courtesy, as we had free time for it, but we cannot go back-and-forth for months to make the styling fit anybody's personal preferences.

All the infrastructure work is there. Actual styling is subjective and a usual rabbit-hole. You should easily tweak the configurations for your needs, and continuing this back-and-forth is not something that we can continue doing forever.

aqnuep commented 1 week ago

There is no whack-a-mole on formatting issues here. The related open conversations are everything. I count 2 format change requests and requests to exclude 4 files from formatting. That is not a huge amount of work.

Please also consider that rebasing a formatting change is a nightmare. We've undone the one-shot formatting, you can adjust the styling as needed, and re-run clang-format on all C++ files then merge it once you're happy with the output. It should be trivial from this point.

MarkCallow commented 1 week ago

the understanding that due to the additional requests we ran out of time and budget to do the clang-format change

Were these "additional requests" related or unrelated to clang-format? If the former what where they?

There is no way to exclude individual files from formatting.

Because clang-format can't run on an entire source tree, all the tools drive it file-by-file.

These statements conflict. What am I missing?

MarkCallow commented 1 week ago

There is no way to exclude individual files from formatting.

What about .clang-format-ignore which I've just discovered at https://clang.llvm.org/docs/ClangFormat.html.

aqnuep commented 1 week ago

the understanding that due to the additional requests we ran out of time and budget to do the clang-format change

Were these "additional requests" related or unrelated to clang-format? If the former what where they?

There is no way to exclude individual files from formatting.

Because clang-format can't run on an entire source tree, all the tools drive it file-by-file.

These statements conflict. What am I missing?

Edit: accidentally pressed the wrong button.

They do not conflict. clang-format, the CLI tool does run file-by-file (unless you run it with a script or indirectly through git clang-format, which is ran for all changed files), but the style sheets (the .clang-format files) apply to entire directories. You cannot change them file-by-file.

The only way to disable clang-format for a single file to either move it to a separate directory for which you disable the styling in the .clang-format file, or you modify the source to surround the content that is not supposed to be formatted with // clang-format off and // clang format on.