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

[Meta] POC: introduce clang-format #1203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Meta] POC: introduce clang-format #1203

wants to merge 1 commit into from

Conversation

Razish
Copy link
Member

@Razish Razish commented Feb 10, 2024

DO NOT MERGE

This is a proof of concept for what introducing clang-format would look like.

Highly destructive for forks, but so much nicer to work with.
I typically work with format-on-save, but have provided a helper script (scripts/format-code.sh) that could also be repurposed by CI to check if PRs adhere to code formatting rules.

Ref #1202

@Razish Razish requested a review from a team as a code owner February 10, 2024 11:53
@Razish Razish changed the title POC: Introduce clang-format POC: introduce clang-format Feb 10, 2024
@Razish Razish changed the title POC: introduce clang-format [Meta] POC: introduce clang-format Feb 10, 2024
@bartrpe
Copy link

bartrpe commented Mar 5, 2024

uh, incorporating that into the forks would be a nightmare beyond belief

I'm afraid at this point it kinda has to stay the way it is. I'm afraid OpenJK reached the point in development where doing full reformatting is just not viable anymore ;_;

I tried incorporating it locally onto various popular forks - EternalJK, GalaxyRP, ZykMod... and there's so much mess and problems with mods that ran bigger changes it's a nightmare. Merge conflicts are all over the place. The problems are often barely readable, diff returns spewage at times. At this point, if you had a mod change too much stuff and rework entire large blocks of code in between unchanged blocks and lines, then the whole file needs full, manual review. Every single file, line by line gets returned as spewage.

I tested it by cloning EternalJK/GalaxyRP/others onto my local drive, and then adding OpenJK as remote. Just go at it with this: git remote add clangFormatTest https://github.com/JACoders/OpenJK.git and then simply fetch the remote branches and try tinkering with it. Results are terrible.

Essentially, if mods introduce deeper changes that are not within completely separate blocks of code, everything turns into a mess.

I'm really afraid it's not viable at this point. If it goes into master then people trying to auto-sync to upstream will encounter terrible mess far beyond most people ability to control.

Hell, honestly, while I could make it work I would die inside trying to fit it into GalaxyRP.

@Razish
Copy link
Member Author

Razish commented Mar 5, 2024

I wonder if it would be easier for forks if they pre-formatted with the same clang-format config before merging.
Would still require large coordination and likely still be an extremely messy merge, but...probably smaller blast radius?
Still not worth it 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants