Godot: Coding standards

[martindzejky]

So after making my first contribution, I think that Godot's codebase really needs some coding standards and a way to enforce them. At least some basics like name conventions, tab indentation, etc.

It would be cool if existing code could be automatically processed (beautified). I could try to do this, just need your opinion on this guys.

[27thLiz]

I think most of the conventions are pretty simple to learn just by observing (and doing the same as) the sourrounding code.
But still, it would be great to have a written style-guide, so +1 :)

On a sidenote, how would the automatic code processing work? Could we also do things such as somehow running the doctool automatically? People often forget to update the docs after messing with exposed functions ^^

[reduz]

if you look at the code, you'll quickly see it has it's own coding
standards, everything is consistent at last

On Fri, Mar 4, 2016 at 1:47 PM, Chuckeles [email protected] wrote:

So after making my first contribution, I think that Godot's codebase
really needs some coding standards and a way to enforce them. At least some
basics like name conventions, tab indentation, etc.

It would be cool if existing code could be automatically processed
(beautified). I could try to do this, just need your opinion on this guys.

—
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/issues/3916.

[reduz]

oh, a style guide would work, if you want to write one it's welcome

On Fri, Mar 4, 2016 at 2:00 PM, Juan Linietsky [email protected] wrote:

if you look at the code, you'll quickly see it has it's own coding
standards, everything is consistent at last

On Fri, Mar 4, 2016 at 1:47 PM, Chuckeles [email protected]
wrote:

So after making my first contribution, I think that Godot's codebase
really needs some coding standards and a way to enforce them. At least some
basics like name conventions, tab indentation, etc.

It would be cool if existing code could be automatically processed
(beautified). I could try to do this, just need your opinion on this guys.

—
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/issues/3916.

[martindzejky]

My point is that when I opened a file in VS, almost always it said that the file had mixed tabs and spaces. There are also missing spaces here and there, etc.

Maybe it's just me, but I love my code nice, clean and consistent. I know it's hard when various people contribute to the project and that's why an automatic method such as astyle (which apparently processes code before committing) would be required.

[reduz]

oh.. I always use tabs, but I guess others might use spaces, never really
enforced that much. I suppose doing a coding style guide will help

On Fri, Mar 4, 2016 at 2:12 PM, Chuckeles [email protected] wrote:

My point is that when I opened a file in VS, almost always it said that
the file had mixed tabs and spaces. There are also missing spaces here and
there, etc.

Maybe it's just me, but I love my code nice, clean and consistent. I know
it's hard when various people contribute to the project and that's why an
automatic method such as astyle http://astyle.sourceforge.net/ (which
apparently processes code before committing) would be required.

—
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/issues/3916#issuecomment-192362366.

[akien-mga]

Writing a code style would definitely be useful. I'm also fond of neat and consistent code; the current code base is _relatively_ consistent as we all try to emulate @reduz's style which is visible everywhere, but writing it down (maybe improving it a bit) would be nice indeed.

Not sure if we should automatically enforce it though; that's open for discussion. Right now I'm doing the watchdog to ensure that we get at least properly indented code (also good git commit logs, which should also go in a kind of style guide I guess).

[OvermindDL1]

If you want to automatically enforce a coding style easily you could just
use clang-format, which by default will reformat the code to follow the
LLVM spec, although you can supply your own spec. It runs on
linux/windows/mac, is part of the clang compiler toolchain or standalone,
and is easy to put into a git pre-commit hook.

On Fri, Mar 4, 2016 at 10:22 AM, Rémi Verschelde [email protected]
wrote:

Writing a code style would definitely be useful. I'm also fond of neat and
consistent code; the current code base is _relatively_ consistent as we
all try to emulate @reduz https://github.com/reduz's style which is
visible everywhere, but writing it down (maybe improving it a bit) would be
nice indeed.

Not sure if we should automatically enforce it though; that's open for
discussion. Right now I'm doing the watchdog to ensure that we get at least
properly indented code (also good git commit logs, which should also go in
a kind of style guide I guess).

—
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/issues/3916#issuecomment-192367663.

[bojidar-bg]

@OvermindDL1 ~ Unfortunately, this would mean that everyone who wants to hack godot would need the clang toolchain installed (unlike me :smile: ).

[OvermindDL1]

Heh, not necessarily, if it does not exist have the script skip it, but the
contributors that do use it (or perhaps the CI) can do it instead. :-)

On Fri, Mar 4, 2016 at 10:35 AM, Bojidar Marinov [email protected]
wrote:

@OvermindDL1 https://github.com/OvermindDL1 ~ Unfortunately, this would
mean that everyone who wants to hack godot would need the clang toolchain
installed (unlike me [image: :smile:] ).

—
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/issues/3916#issuecomment-192374071.

[martindzejky]

What about the existing code? It is relatively consistent but there are some errors, which could be automatically processed...

Also +1 for clang-format.

[OvermindDL1]

Indeed, I'd think a CI step should be to run some auto-formatter over the
whole source every run and commit the change as an auto-formatted change.

Clang-format should not create errors as it uses the compiler's AST itself
to do the reformatting. If the code compiles, it should work without issue.

On Fri, Mar 4, 2016 at 10:56 AM, Chuckeles [email protected] wrote:

What about the existing code? It is relatively consistent but there are
some errors, which could be automatically processed...

Also +1 for clang-format.

—
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/issues/3916#issuecomment-192381395.

[martindzejky]

I have created a very simple project to test the clang-format on Travis CI: Github repo and Travis CI. It works fairly well, when it receives an update, it formats the code, compiles, commits and pushes back to the repository.

Did you mean something like that?

[neikeq]

I agree that we need code standards. Godot has a consistent formatting, but there are times when one can get confused. Some simple examples:

• I find code where the operator = is surrounded by spaces, and parts where it's not:

bool running=false;
int offset = 0;

• Should we have an empty line after a block opening? It's also inconsistent sometimes (the first one seems the most common though):

if (!done) {

    do_something();
}

if (button->is_pressed()) {
    button->set_pressed(false);
}

• Space after comma?:

_fix_object_paths(res.operator->(), root, save_path);
_find_and_save_resource(res,processed,flags);

• Space after parenthesis opening and before closing?

memnew( ResourceImportMetadata );
memnew(VBoxContainer);

• Is there a limit for line width in characters?

[OvermindDL1]

Yes, although only committing if a change actually happened during the
formatting, which I guess it would not do anyway...

On Fri, Mar 4, 2016 at 2:05 PM, Chuckeles [email protected] wrote:

I have created a very simple project to test the clang-format on Travis
CI: Github repo https://github.com/chuckeles/clang-format-travis-test
and Travis CI https://travis-ci.org/chuckeles/clang-format-travis-test.
It works fairly well, when it receives an update, it formats the code,
compiles, commits and pushes back to the repository.

Did you mean something like that?

—
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/issues/3916#issuecomment-192466427.

[martindzejky]

Well, if it doesn't change anything, it doesn't commit anything, so there's nothing to push...

[martindzejky]

This would be awesome if something like this would get integrated ^_^

[martindzejky]

@neikeq Exactly! That's why I want to do something like this, automatically. Because these are just little things that slip by when developing code.

[est31]

A written down code style would certainly be an improvement. I too often wonder which way something should look like, and then I just write it _some_ way.

I like the general idea of an automated way of ensuring the code style, but there are some open questions:

• How to get there? Gradually, slowly, or with one big huge commit?
• Exclude or include imported dependencies like openssl?
• Require it from the commiter (lock-in to LLVM thats bad IMO) or some other way? It can be managed like classes.xml, people update it in their PRs.

[martindzejky]

Just as a quick test, I run clang-format on every .h and .cpp file in the codebase locally and then compiled Godot. Compilation went smoothly without any problems.

[akien-mga]

Just as a quick test, I run clang-format on every .h and .cpp file in the codebase locally and then compiled Godot. Compilation went smoothly without any problems.

And just for the reference, how many megabytes of diff? :D

[martindzejky]

Using find \( -name "*.h" -or -name "*.cpp" \) -print0 | xargs -0 clang-format -i -style="{BasedOnStyle: google, UseTab: ForIndentation}" in the repo and then doing git diff --shortstat:

2134 files changed, 557831 insertions(+), 563510 deletions(-)

[martindzejky]

Configuring the coding style needs a little work too :laughing:

[akien-mga]

Yeah that's what I feared. Doing such a massive change would basically break _all_ pull requests. So we need a very strong consensus (and especially approval of the proposed style by @reduz and @punto- before doing any such mass change).

[neikeq]

I am not against using automation tools, but I think we could do just fine by creating the style guide and pointing contributors to it. Perhaps then we could use the automated formatting very occasionally, when we are sure it won't create conflicts with existing PRs.

[martindzejky]

Doing such a massive change would basically break all pull requests.

Oh, I didn't think about that, you're right.

[martindzejky]

And where to put the style guide? godotengine/godot-docs/tree/master/contributing maybe?

[est31]

It can be introduced gradually to the codebase, on a per file or per directory basis, like with the Theseus ship :)

[akien-mga]

And where to put the style guide? godotengine/godot-docs/tree/master/contributing maybe?

Yes, in a style_guide.rst file or something. But we should probably start working on it in an Etherpad before putting it in the docs, that would be easier to handle.

[neikeq]

I normally find this info in the CONTRIBUTING.md file. Here is an example.

[est31]

systemd has it in a separate file: https://github.com/systemd/systemd/blob/master/CODING_STYLE

[Marqin]

For reference: https://www.kernel.org/doc/Documentation/CodingStyle

[kirilledelman]

When I was contributing to ThreeJS ( WebGL library for HTML5 ), they had a nice formatting guide. I think Godot would benefit from something like this: https://github.com/mrdoob/three.js/wiki/Mr.doob%27s-Code-Style%E2%84%A2

[neikeq]

If it's done on a separate file, we should still reference it from _CONTRIBUTING.md_

[Marqin]

Instead writing tons of Readmes, why not just write clang-format config?

[Marqin]

Most of IDEs/editor have support for clang-format, it's used by big companies andwhat's important - it can be easly integrated in Travis PR checks.

[neikeq]

I agree we should add clang-format but IMO we still should have a small readme about code style :P

[martindzejky]

How about this:

Write a written coding style and contribution guidelines.
Have people use clang-format or some other automatic formatting tool on the files they change (or just the lines maybe?).

[Marqin]

I've created #4741 that adds travis-ci check for trailing whitespace.

[Geequlim]

Maybe we could write down the standard code rules into an EditorConfig configuration file under the project root directory as most of editors and IDE can use it.

[Marqin]

@Geequlim I don't think we should use some new, unknown program, when clang-format is battle-tested by tons of HUGE projects(Google, Mozilla, LLVM, etc.), has plugins for most of IDEs and great CMDline tool.

[Marqin]

for reference: https://clangformat.com/ - this tool helps building custom .clang-format config

[akien-mga]

I think it would now be the right time to introduce code style guidelines. Since enforcing a code style would imply a huge diff, it's best to have it done right now before the big 3.0 refactoring IMO. This way we'd have "good" commits that are after the style guide application but before the refactoring, that we can use to bisect refactoring-induced issues more easily.

The main question is "what style". And it mostly depends on @reduz, since so far we took his own style as the reference and tried to emulate it. If we try to make things a bit more rational, by enforcing e.g. one space on each side of an assignment, or no space, we need to ensure that the first concerned person (still writing 90% of the code) will like it and respect it.

[SuperUserNameMan]

IMO the style should simply be the main style of @reduz.
when inconsistencies exists, what @reduz use the most recently, is the main rule.

[volzhs]

I have been tried to keep style with previous (actually @reduz) codes also.

[hubbyist]

Doing big code styling as the opening ceremony of 3.0 has a warm feeling of its own. Like firing cannons for a new era of automatic code styling. :smile:

[akien-mga]

I made a heavily commented (see all //? comments) commit with a first proposal for a code standard we could try to enforce. Please don't hesitate to add comments there on the relevant lines so that we can discuss this further on real use cases.
https://github.com/akien-mga/godot/commit/7cd11e1a41f1cce91c39b88b3ae38a6a65c46d87

(Note that it's on my fork, not applied to upstream yet of course)

[akien-mga]

BTW, I would avoid applying any code standard to the C++ files until the gles3 branch is merged into master, otherwise it will be a pain to solve the merge conflicts.

[akien-mga]

Fixed with 5dbf1809c6e3e905b94b8764e99491e608122261 :)