godot: Let's talk about the inconsistencies in the code-style

But we’re not making a plane onboard controller, we’re making a game engine.

Both are quite complex and need to be readable, maintainable, because they need to function long-term. Sure, lives hopefully don’t depend on Godot in the same way, but peoples livelihood may if you expect users to use Godot for actually earning their income.

It’s a question of balance between code quality and being welcoming to developers

Agreed. But:

that have different tastes regarding code style

As a potential new contributor, “we have no style” sounds more offputting to me than “we enforce a style that you don’t like”. I can live with doing formatting according to your whims, if it is consistent, and actually would prefer that over a wild mix of styles all over the place.

And I disagree strongly on the braces. I already saw bugs that wouldn’t have been possible with braces in conditionals, and I already debugged one I caused myself because I modified existing code and missed the missing braces. Comments and stuff are nitpicking, yes, but for braces, always having them trades a little more verbosity in some cases against eliminating a whole class of bugs. Far worth it, IMO.

I think we can enforce a more strict style, we already do to some extent (more than the clang-format, PR-reviewers usually point other things too).

Regarding single line ifs I prefer no braces if it can be on the same line, otherwise always use braces even if it’s a single line. E.g.:

// Simple enough to be a single line.
if (!is_inside_tree()) return;

// Not on the same line, use braces.
if (something && something_else || something_other) {
    do_something_more();
}

I’d like to throw one under the comments header. The engine code needs quite a bit more commenting in the source code, so new people (we’re aiming to allow anybody to contribute, right?) can look at a section of code and go “That’s what that’s doing”. Right now though, I’ve noticed (and maybe I’m not looking enough, but) a big lack of comments within the code describing what everything’s doing, which forces people reading the code to have to look at it and make a guess as to what’s happening.

Now I won’t say from now on everything has to be commented, but I’d like there (at least) to come times where we go through and see what needs some in-depth comments. And no, I don’t believe self-documenting function names are enough in this case.

Just to clarify, this is in regards to the engine’s source code, not the documentation for GDscript over at godot-docs. I’d like to see at least simple one-liners describing certain sections of code.

@groud

We’re a community driven project that involves a lot of different people with different tastes regarding code style. Let’s not force people to adopt one as it does not really impact the tm project maintainability.

Following that logic, we should just drop clang-format and let people write however they like as long as it’s legible and compiles.

In addition to C++ style, we should also adopt some rules for texts that appear in the editor. Currently:

• Some tooltips and error messages end with a period, but not all.
• Most buttons, menu actions and undo/redo actions are written “Like This”, but a few use casing “Like this”.
• Some texts use contracted forms (“won’t”), some don’t (“will not”). I think using the contracted form is better, as it’s shorter.
• Some texts use simple quotes, others use double quotes. I think double quotes make more sense, as they’re the de facto character to quote things in English.

In addition to the current doc writing guidelines, we could also take inspiration from the Fluent writing style guidelines.

There’s also a small inconsistency when it comes to property hints. Some of them use spaces after commas, but most don’t:

I tend to prefer the compact form without spaces, which makes it clear that the string is a property hint, and not several extra arguments to the ADD_PROPERTY() or set_custom_property_info() method call. But it’s true that it’s an inconsistency with the style enforced within C++ code, so if we want to enforce a space after comma in property hints, that’s fine with me too.

Ideally, you don’t have to ask them because the CI already fails if code isn’t formatted correctly.

Well, following yours, we should start including a code quality static checks. But we’re not making a plane onboard controller, we’re making a game engine.

IMO, the workflow is good as is. We ensure that merging is not getting messy with too many aesthetic changes while still letting people make their own decision on the code they are working on… It’s a question of balance between code quality and being welcoming to developpers that have different tastes regarding code style.

Also, to illustrate my point, I generally don’t agree with your proposals. They are some places where brackets are just too verbose, one-line functions are approriate (in header files sometimes). Also, regarding the comments, I actually don’t care. So I would not force people to adopt a rule on that. It’s already hard to make good code I’m not going to bother people so that they put a capital and a point in their comments.

@dreamsComeTrue I think it’s for readability. If that’s the case, it doesn’t make much sense; better place the brace in a new line, right?

There’s also a small inconsistency when it comes to property hints. Some of them use spaces after commas, but most don’t:

https://github.com/godotengine/godot/blob/5441aaf768d6dd4c3d8465e6b340ae38ddc7db1d/core/io/file_access_network.cpp#L515

https://github.com/godotengine/godot/blob/5441aaf768d6dd4c3d8465e6b340ae38ddc7db1d/main/main.cpp#L783

We should probably pick one style or the other. I’m not sure which style would be best; they’re more readable when separated with spaces, but this makes them look like an actual string that would be displayed to the user. On the other hand, not using spaces makes it clearer that the property hint string is only used for internal purposes.

This inconsistency is also present in FileDialog filters (notice the space before the semicolon):

https://github.com/godotengine/godot/blob/5441aaf768d6dd4c3d8465e6b340ae38ddc7db1d/editor/editor_feature_profile.cpp#L897

https://github.com/godotengine/godot/blob/5441aaf768d6dd4c3d8465e6b340ae38ddc7db1d/editor/project_manager.cpp#L376

Ifs: Agree, braces are nice.

Functions: I have to agree with @neikeq, if we’re going to take up that vertical space anyway, we should use it! I haven’t raised an issue about this before because I figured it’d be a lot to change.

Comments: Capitalization is good. Additionally, they should always start with spaces. // Test instead of //Test. This helps differentiate commented out code from actual English comments. I don’t personally care about having punctuation or not.

Comments would probably be easiest to fix without upsetting any contributors who prefer their code looks a certain way. I think everyone would prefer either // Test or // Test. over //test

@LinuxUserGD I don’t see how the suggestions above would harm performance, they are purely aesthetic as far as I know.

Besides, I don’t think anyone should go straight optimization > readability, but instead, a fine balance between the two.