HerculesWS
Standardise Code Formatting with clang-format and Fix Spacing Inconsistencies
Pull Request Prelude
- [x] I have followed proper Hercules code styling.
- [x] I have read and understood the contribution guidelines before making this PR.
- [x] I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.
Changes Proposed
This commit addresses spacing inconsistencies across the codebase and introduces clang-format to enforce a uniform coding style. By integrating clang-format, we ensure consistent formatting across all files, improving code readability and maintainability.
- Resolved inconsistent spacing in various files.
- Applied clang-format to standardise code formatting.
- Added configuration (if applicable) to automate formatting enforcement.
Issues addressed: #3130
We've had these kind of commits several times in the last 15-20 years ever since eAthena. They break git blaming severely, or rather make it extremely annoying. I know it seems desirable to fix the code-style all in one go, but this is not the way. It should be a slow process, for whenever people touch code, it should be (almost) perfectly formatted.
Also this setting changed the function pointer defintions in the interface in a way that we do not consider our code style:
int (*init) (bool minimal);
->
int (*init)(bool minimal);
Although I have to agree that it's a bit inconsistent with normal function signatures not having a space between name and the parameter list, but function pointers are special.
EDIT:
Also seems to introduce breaking changes:
-CMDLINEARG_DEF2(run-once, runonce, "Closes server after loading (testing).", CMDLINE_OPT_NORMAL);
-CMDLINEARG_DEF2(api-config, apiconfig, "Alternative api-server configuration.", CMDLINE_OPT_PARAM);
-CMDLINEARG_DEF2(net-config, netconfig, "Alternative subnet configuration.", CMDLINE_OPT_PARAM);
+CMDLINEARG_DEF2(run - once, runonce, "Closes server after loading (testing).", CMDLINE_OPT_NORMAL);
+CMDLINEARG_DEF2(api - config, apiconfig, "Alternative api-server configuration.", CMDLINE_OPT_PARAM);
+CMDLINEARG_DEF2(net - config, netconfig, "Alternative subnet configuration.", CMDLINE_OPT_PARAM);
also not sure if this is breaking as well:
-#define SEND_LOGIN_ASYNC_DATA(name, data) aloginif->send_to_server(fd, sd, API_MSG_ ## name, data, sizeof(struct PACKET_API_ ## name), proxy_flag_login)
-#define SEND_CHAR_ASYNC_DATA(name, data) aloginif->send_to_server(fd, sd, API_MSG_ ## name, data, sizeof(struct PACKET_API_ ## name), proxy_flag_char)
-#define SEND_MAP_ASYNC_DATA(name, data) aloginif->send_to_server(fd, sd, API_MSG_ ## name, data, sizeof(struct PACKET_API_ ## name), proxy_flag_map)
+#define SEND_LOGIN_ASYNC_DATA(name, data) aloginif->send_to_server(fd, sd, API_MSG_##name, data, sizeof(struct PACKET_API_##name), proxy_flag_login)
+#define SEND_CHAR_ASYNC_DATA(name, data) aloginif->send_to_server(fd, sd, API_MSG_##name, data, sizeof(struct PACKET_API_##name), proxy_flag_char)
+#define SEND_MAP_ASYNC_DATA(name, data) aloginif->send_to_server(fd, sd, API_MSG_##name, data, sizeof(struct PACKET_API_##name), proxy_flag_map)
(see the ##)
EDIT2: Also doesn't seem to adhere to our character limit per line at all:
- if (SQL_ERROR == SQL->Query(inter->sql_handle,
- "INSERT INTO `%s` (`char_id`,`char_name`,`party_id`,`min_level`,`max_level`,`type`,`flags`,`message`) VALUES ('%d','%s','%d','%u','%u','%d','%d','%s')",
- adventurer_agency_db, char_id, char_name_esc, party_id, entry->min_level, entry->max_level, entry->type, flags, message_esc)) {
+ if (SQL_ERROR == SQL->Query(inter->sql_handle, "INSERT INTO `%s` (`char_id`,`char_name`,`party_id`,`min_level`,`max_level`,`type`,`flags`,`message`) VALUES ('%d','%s','%d','%u','%u','%d','%d','%s')", adventurer_agency_db, char_id, char_name_esc, party_id, entry->min_level, entry->max_level, entry->type, flags, message_esc)) {
Another big no-no for this: Maintaing a strongly customized fork of Hercules will make merging this upstream change pure suffering, as pretty much every possible change you did will likely conflict.
I do realise the commit is large and spans everything, and I wasn't sure if committing it would even make sense, regardless of git blame issues. Though it does seem odd to enforce a style on users that do a push request and not a properly maintained style in the master branch, yes I did read that it should be handled when it is handled, but going by that logic it may never be handled.
EDIT2: Also doesn't seem to adhere to our character limit per line at all:
- if (SQL_ERROR == SQL->Query(inter->sql_handle, - "INSERT INTO `%s` (`char_id`,`char_name`,`party_id`,`min_level`,`max_level`,`type`,`flags`,`message`) VALUES ('%d','%s','%d','%u','%u','%d','%d','%s')", - adventurer_agency_db, char_id, char_name_esc, party_id, entry->min_level, entry->max_level, entry->type, flags, message_esc)) { + if (SQL_ERROR == SQL->Query(inter->sql_handle, "INSERT INTO `%s` (`char_id`,`char_name`,`party_id`,`min_level`,`max_level`,`type`,`flags`,`message`) VALUES ('%d','%s','%d','%u','%u','%d','%d','%s')", adventurer_agency_db, char_id, char_name_esc, party_id, entry->min_level, entry->max_level, entry->type, flags, message_esc)) {
Regarding this that's a simple change of ColumnLimit, I set it to 9999 to avoid it breaking arbitrarily, otherwise it would have introduced additional line breaks, this could be a reason.
Though int_rodex.c line 287 still has the following with this very config.
if (SQL_ERROR == SQL->Query(inter->sql_handle,
"INSERT INTO `%s` (`sender_name`, `sender_id`, `receiver_name`, `receiver_id`, `receiver_accountid`, `title`, `body`,"
"`zeny`, `type`, `is_read`, `sender_read`, `send_date`, `expire_date`, `weight`) VALUES "
"('%s', '%d', '%s', '%d', '%d', '%s', '%s', '%" PRId64 "', '%d', '%d', '%d', '%d', '%d', '%d')",
rodex_db, sender_name, msg->sender_id, receiver_name, msg->receiver_id, msg->receiver_accountid, title, body, msg->zeny, msg->type, msg->is_read == true ? 1 : 0, msg->sender_read == true ? 1 : 0, msg->send_date, msg->expire_date, msg->weight)) {
Sql_ShowDebug(inter->sql_handle);
return 0;
}
Though this has little to do with "character limit" as you say it and more of a stylistic choice I believe. Same file also has the below, I do realise there are fewer characters but still. What is the character limit then? *Note it's probably more specific to sql statements.
if (SQL_ERROR == SQL->Query(inter->sql_handle, "SELECT `zeny`, `type` FROM `%s` WHERE `mail_id` = '%" PRId64 "'", rodex_db, mail_id)) {
As for this argument:
Another big no-no for this: Maintaing a strongly customized fork of Hercules will make merging this upstream change pure suffering, as pretty much every possible change you did will likely conflict.
That is always up to the fork-ers, it is a breaking change indeed, so it wouldn't be categorised as a v0.1 or v0.0.1. I don't see major distributors considering this when releasing updates other than with "includes breaking changes". People should be diff/patching their own version anyways, otherwise any /src/* change will cause issues.
auto formatting tools always may change something in style into wrong way and no way to fix it. better use some format validation tool and only for changed lines in commits or pull requests.
How about making this only apply to places that are not normally changed by forkers, like src/api, src/common, src/config, src/plugins and src/test, the rest can be changed slowly later and new code that comes in via PRs or commits should automatically apply the new formatting
How about making this only apply to places that are not normally changed by forkers, like
src/api,src/common,src/config,src/pluginsandsrc/test, the rest can be changed slowly later and new code that comes in via PRs or commits should automatically apply the new formatting
That certainly would be an idea. Though I'm not sure if everyone agrees with the clang-format implementation I used, or even with the tool. I did try to keep it as close to the current format as possible.
In regards to
-CMDLINEARG_DEF2(run-once, runonce, "Closes server after loading (testing).", CMDLINE_OPT_NORMAL);
-CMDLINEARG_DEF2(api-config, apiconfig, "Alternative api-server configuration.", CMDLINE_OPT_PARAM);
-CMDLINEARG_DEF2(net-config, netconfig, "Alternative subnet configuration.", CMDLINE_OPT_PARAM);
+CMDLINEARG_DEF2(run - once, runonce, "Closes server after loading (testing).", CMDLINE_OPT_NORMAL);
+CMDLINEARG_DEF2(api - config, apiconfig, "Alternative api-server configuration.", CMDLINE_OPT_PARAM);
+CMDLINEARG_DEF2(net - config, netconfig, "Alternative subnet configuration.", CMDLINE_OPT_PARAM);
I have to check. I believe clang-format treats it as 2 variables being subtracted hence the change. This can be avoided by wrapping the lines in
// clang-format off
CMDLINEARG_DEF2(run-once, runonce, "Closes server after loading (testing).", CMDLINE_OPT_NORMAL);
CMDLINEARG_DEF2(api-config, apiconfig, "Alternative api-server configuration.", CMDLINE_OPT_PARAM);
CMDLINEARG_DEF2(net-config, netconfig, "Alternative subnet configuration.", CMDLINE_OPT_PARAM);
// clang-format on
While a little ridiculous, this avoids issues. Though it would have to be approved to begin with.
We've had these kind of commits several times in the last 15-20 years ever since eAthena. They break git blaming severely, or rather make it extremely annoying. I know it seems desirable to fix the code-style all in one go, but this is not the way. It should be a slow process, for whenever people touch code, it should be (almost) perfectly formatted.
You're absolutely right that large formatting commits can disrupt git blame and historical tracking. To address this, we can:
-
Use .git-blame-ignore-revs to mark this commit as a formatting-only change, preserving git blame usability (Git documentation supports this).
-
Adopt an incremental approach: enforce formatting only on files touched in future PRs and gradually expand coverage.
-
Pair clang-format with pre-commit hooks to automate formatting for new changes, avoiding mass reformatting.
This balances consistency with historical traceability.
Another big no-no for this: Maintaing a strongly customized fork of Hercules will make merging this upstream change pure suffering, as pretty much every possible change you did will likely conflict.
While forks may face temporary merge conflicts, they can easily adopt the same clang-format rules to align their custom code. Holding back improvements for fear of disrupting forks would harm the project’s long-term maintainability. Consistent formatting benefits everyone by reducing cognitive overhead and easing contributions. We should prioritize code health over short-term merge concerns -forks can adapt with minimal effort using the provided config.
auto formatting tools always may change something in style into wrong way and no way to fix it. better use some format validation tool and only for changed lines in commits or pull requests.
Auto-formatters can sometimes misinterpret intent, but clang-format’s flexibility helps here:
Customize the config rigorously to match our style (e.g., line breaks, braces) and test it on critical code sections.
Add a CI check that validates formatting without auto-applying it, allowing manual fixes for edge cases.
Use // clang-format off directives for code blocks requiring manual control. This ensures automated consistency while preserving human oversight where needed.
and yes, this is great and better than this cringe stuff on our codding style guideline:
Hercules follows the same guidelines as the Linux Kernel (because Linus is pretty much always right, and he doubtlessly knows what he's doing)