Restore the .clang-format file and explain how to use it.#139
Restore the .clang-format file and explain how to use it.#139aknrdureegaesr wants to merge 7 commits into
Conversation
|
As I found out the hard way and @Chris-AC9KH also mentioned in #141 (comment) :
So the recommendation to run We should do this only once, to establish a baseline that's clean, as far as coding style is concerned. |
|
clang-format has already been run on all files except the JS8 checker files. This was done during the rebuild of the code tree, along with fixing several problems I found in the code. I don't know how I can make this any more clear - you are using the wrong format for your clang-format implementation. Nobody uses WebKit on C++ code, why are you trying to do that? Do you not understand how clang-format works? There is over 30 different implementations of it in Xcode alone. So I'm not really sure what you're trying to do here, and I don't think you know yourself. If you wanted to take part in it you should've showed up when the code tree was being rebuilt. But you're late to the party now. I got better things to do than deal with old news that's trying to re-invent the wheel. So I'm going to state this here once and then I'm done with it: |
|
@wmiler @aknrdureegaesr @rruchte there's a LLVM standard clang-format configuration file in the tools directory in this branch, generated from Xcode. You can use it to get rid of that cobbled up WebKit one you're using, that is trying to implement some new coding style never seen before by the human race. Tested on MacOS with Xcode 26.0.1, and Windows 11 with Visual Studio 2024 it doesn't attempt to reformat any of the current codebase that has already been formatted. I don't know if linux will have the same results because linux does not use the same clang-format as MacOS and Windows. Maybe it will, maybe it won't. But you can try it. If it works on linux, then you can put it in the tools directory for linux developers to use. And include your developer documentation on how to use it in the developer guide. I suggest using it on ONLY one file at a time, whatever is being worked on by the developer. NEVER apply it to the entire codebase. Add .clang-format to .gitignore so linux developers can copy it to the root of the file tree and then call it with: Adding it to .gitignore will prevent it from getting into the root of the source tree when linux developers submit PR's. MacOS and Windows developers don't need it since these tools are built into their IDE's, and both Mac and Windows use the same LLVM standard if you select that in their respective IDE's. They don't need any .clang-format configuration file. This should fix your code formatting standard issue (assuming it works on linux) without reformatting the codebase (again). From that point it can be looked at on a per-PR basis to see if has been used pre-PR. |
😆 Thanks for any appreciation of the work @wmiler and I put into it. Seriously: It is more important to me we use a uniform coding style in the first place than which coding style that is. I see no reason to tug that file into a subdirectory, rather than the project's root directory where |
Would the existence of that file in the root directory hurt Windows or Mac people? |
Ok, in the process. First result: The Debian Trixie |
|
The first .clang-format file was half WebKit, half something else. It didn't meet any standard I've ever seen. So, what problem does Debian trixie have with the standard LLVM format? It can't do LLVM standard? So in other words, if you run clang-format on Linux it won't work without a .clang-format file? So you can't use |
|
@aknrdureegaesr I also fail to understand why you are using clang-format 18. clang-format 21 is in Xcode 26.0.1 and that will be updated to clang-format 23 at the next Xcode point release. clang-format 18 is seriously out-of-date and does not support some of the newer C++ standards that have come out since it was released. |
clang-format 20 is the latest available in the Ubuntu 24.04 repos. clang 23 isn't released anywhere yet. clang-18 is the default version. Just because you can be on the "latest and greatest", doesn't mean you should be. |
This shows no issues with any of the cpp files. Looks like you would need to do some work on JS8_Include/commons.h tho, haha. Oh, and JS8_Main/JS8MessageBox.h but that's just one line. Chris's clang-format file doesn't work for me tho, it's throwing a lot of this'n'that missing. |
|
I used four-space indent when I did the recent code overhaul. But anyway, that goes to show that clang-format is not the same between the linux versions and what MacOS and Windows uses. You can run clang-format --dump-config with your desired style and generate a .clang-format file, which is what I did with clang-format 21 in Xcode 26. If that file doesn't work with linux, then linux clang and MacOS/Window clang are not compatible. Which makes sense because the clang compiler won't build JS8Call on linux, but it does on both MacOS and Windows. |
Once at it, improve formatting of CONTRIBUTOR_GUIDE.md.
ef39389 to
639c6c8
Compare
|
Might not have v20 in the Ubuntu 22.04 repos. Which is what we use on the CI runs. And, NO, you may not change it to ubuntu-latest (or-24.04) runner, because that will totally break AppImage generation. |
|
The 2022 Ubuntu we use has this to offer: We could potentially use a different OS just for code checking. Before we go there, let's see how well |
|
Yeah, it was a really long time ago! I can whip up a dry-run runner over on my repo and give it a whirl there. And it would actually make more sense as a separate runner for tooling-related runs anyway. |
|
@aknrdureegaesr Andreas, this is the way you want to do this, as a separate yaml file. test-clang-format.yml name: Format check on Linux Ubuntu
run-name: ${{ (github.event.pull_request.title) || 'CI' }} Format check on Linux Ubuntu
on:
[pull_request]
jobs:
clang_format_check:
runs-on: ubuntu-latest
name: Check Code Style
strategy:
fail-fast: false
steps:
- uses: actions/checkout@v5
- name: Install minimum required dependencies (Ubuntu)
run: |
sudo apt-get update
sudo apt-get install -y clang-format-20
- name: Run clang-format dry-run
run: |
clang-format-20 --dry-run --style='{BasedOnStyle: LLVM, IndentWidth: 4}' JS8*/*.h JS8*/*.cpp |
|
Output from the above test yml: https://github.com/wmiler/JS8Call-improved/actions/runs/21218886313/job/61047916993 |
|
I don't recall that I ever ran clang-format on those JS8_include files unless it was modified by the recent code changes to mainwindow.cpp and the doxy hooks. If it wasn't touched by that the file was likely not formatted. |
|
I'm marking this stale because of several reasons:
As long as the code remains readable and well-written, there is no need to enforce any sort of universal formatting stye, as it does nothing but create friction among developers |
|
Closing this PR as wontfix. |

With 4a7a955, the
.clang-formatfile was removed, I presume, accidentally.As part of the effort for #138 , this restores that file and also explains how to use it.