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

Code formatting #223

Closed
ZombieRaccoon opened this issue Aug 22, 2021 · 7 comments
Closed

Code formatting #223

ZombieRaccoon opened this issue Aug 22, 2021 · 7 comments
Labels
discussion Meta talk and feedback enhancement New feature or request

Comments

@ZombieRaccoon
Copy link

I'd like to start off with a subjective statement: during development it's convenient to just ask the IDE to automatically format the code as the maintainer wills it. The main advantages are having a codebase that's completely uniform in terms of formatting and no longer having to deal with manually performing the task.

Building on the initial statement, would you be against introducing some form of automatic code formatting to the codebase? I can recommend clang-format, which I've had a positive experience with. It's highly customizable and has relatively widespread integration across various IDEs.

If the initial statement doesn't ring any bells then feel free to close the issue.

mikke89 added a commit that referenced this issue Aug 23, 2021
@mikke89
Copy link
Owner

mikke89 commented Aug 23, 2021

Thanks for the suggestion, I've added one now! It might still need some tweaking but should be a good start at least.

@mikke89 mikke89 added discussion Meta talk and feedback enhancement New feature or request labels Aug 23, 2021
@ZombieRaccoon
Copy link
Author

I'm glad you liked the idea! I see that you've had quite a bit of fun with the customization. May I suggest that the entire codebase be reformatted at some point in order to establish a stable baseline?

The following PowerShell script, for example, would do the job:
Get-ChildItem -Recurse -Include @("*.c", "*.cpp", "*.h", "*.hpp") | ForEach-Object { clang-format.exe -i --verbose $_.FullName }

One could of course easily imagine a similar Unix command as well.

@mikke89
Copy link
Owner

mikke89 commented Aug 23, 2021

Hah, thanks. Yes, I went through the options one by one and tried to find something that fit the existing code.

I'll consider formatting the code base. My concern is that it will mess up git blame and history for the foreseeable future. But maybe we might as well take that pain now. Hmm.

@ZombieRaccoon
Copy link
Author

Trying not to mess up git blame is a completely valid concern. I'd say that the choice is between messing it up gradually or all at once. The former potentially makes individual reviews cluttered with formatting changes but leaves unchanged parts of the codebase alone, while the latter establishes a baseline with no additional formatting required in later reviews but ends up bothering parts of the code that haven't been modified in ages. In my opinion there's no clear-cut winner here, but in case of a mature library such as this one and amid concerns of losing valuable information perhaps the former could be considered a more fitting policy. In any case, it's hard to pick and the ultimate choice would most likely depend on one's usage patterns of version control history.

@barotto
Copy link
Contributor

barotto commented Aug 24, 2021

If I may, RmlUi's code base is already well formatted and generally consistent.
I don't think polluting a valuable tool like the git history would be a good idea at this point...
Maybe I would use a code formatter only to check that new code follows a general rule.

The thing I would really look into though is making sure there's no symbol masking in member functions, but maybe this is more the domain of a static analyzer?

@andreasschultes
Copy link
Contributor

I think also this should only be used for new or changed code.
In addition a .clang-tidy file could be useful.

@mikke89
Copy link
Owner

mikke89 commented Aug 24, 2021

Alright, I think the consensus is to leave it be, and only do formatting for new code. I'm fine with that, I don't really have any problems with the current formatting style of the library.

As for static analysis and clang-tidy. I have tried this on the code base, and to be honest it's just way too noisy at this point. Somebody would have to do a big cleanup job - which itself can be risky in terms of introducing new bugs - or we would pretty much have to disable a lot of the checks.

@mikke89 mikke89 closed this as completed Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Meta talk and feedback enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants