Do we have an official coding style?

[fredizzimo]

As I finally will start to write some actual code for QMK, as a part of the effect system described in #717. I started to wonder about the coding style.

We seem to use many different styles, even the quantum.c file is not consistent. The codebase has a mixture of at least these

// Brackets on the same line
void t() {
}
// Brackets on a new line
// vs
void f()
{
}

// Indentation of two spaces
void t() {
  // Something here
}
// Indentation of four spaces
// vs
void f(){
    // Something here
}
// At least I don't think we uses a mixture of tabs and spaces (except in the Makefiles, 
// where it matters)

// Function names and variable names seems to be mostly consistent, and use lower
// caps with underscore as a word separator. 
function_names_like_this();
this_is_a_variable=5;

// But in some files we have this (even mixed with the previous form)
functionNameLikeThis();

// At least I think defines are consistent
#define DEFINE_SOMETHING

// Enums are mostly defined like this
enum a_enum
{
    PREFIX_VALUE1,
    PREFIX_VALUE2
};

// But this also exist in the code base, where both the prefix is missing and the 
// identifier is written in lower case. Sometimes only the prefix is missing
enum an_enum{
    value1,
    value2
};

// Structs are mostly declared like this
typedef struct {
} a_struct_t;

// But sometimes this is used, where you are forced to repeat the struct keyword
// whenever it's used
struct aStruct {
};

// Short comments like this
/* Or like this */

For the reference, the only coding standards that the TMK firmware have is the following

Doesn't use Tab to indent, use 4-spaces instead.

It would be nice if we had an official standard for at least these common cases. I'm not suggesting that we change existing code, but new code should probably follow a common standard.

On a different, but somewhat related note. The QMK firmware has to use either GPLv2 or GPLv3, due to the TMK roots. So we should probably make it fully compliant, by adding the appropriate headers and add a "COPYING" file. This is something that has bothered me for some time, and today I saw this pull request for TMK, which reminded me about it again.

[IBNobody]

Please... No... Wars have been started over less.

[fredizzimo]

I'm not really looking for a debate, personally I'm happy with whatever @jackhumbert or someone else tells us to use.

I would just like to see some sort of consistency, but if the coding style is "write in whatever style you personally prefer", I can accept it too.

[jackhumbert]

Definitely agree about the GPL stuff - that's been something we've been needing to address for a while. Maybe that should be a separate issue where we can spend some more time discussing what we need to do for compliance.

For the coding style, let's just run with OTBS, lowercase + underscored variables, uppercase + underscored enums, and spaces for indents. I'm not too worried about indents though, and we probably don't need to worry about what people do inside their own keymap.

[IBNobody]

I loathe OTB. :P Like I said, it is a very touchy subject. Why not just leave it as is?

[fredizzimo]

@jackhumbert Thanks

Just one more question, the Wikipedia page doesn't make it fully clear if you should put the brackets on the same line for functions, or if it's just for control statements.

And yes, I agree, we shouldn't worry about the coding style for keymaps.

@IBNobody
The style suggested makes sense, since it seems like the original code written by TMK, also followed it, with the brackets for function definitions on different lines.

[jackhumbert]

Opening brackets on the same line for functions and control statements :)

OTBS/K&R is pretty standard in C from what I've seen :/

[IBNobody]

Sure, but what would happen if we just kept doing what we are doing? I do not like to code in OTB, but I don't begrudge anybody else their style. :P

Allman is standard too. NASA uses it in their C guidelines.
http://homepages.inf.ed.ac.uk/dts/pm/Papers/nasa-c-style.pdf

[jasonm23]

K&R 👍

[jasonm23]

Although I have no strong opinion on this. I think compliance with TMK and perhaps do something more to promote that style in the contribution docs

[DidierLoiseau]

Enforcing a consistent style really improves readability and avoids that people reformat code based on their own preferences.

Also, it might be an OCT, but I find it very upsetting when the style is inconsistent in a single file or even across files, especially indentation levels.

Coming from the Java world I can't really make a recommendation for C, as the standards seem to be quite different.

I think however that the code should not be reformatted all at once, as this would break the git blame feature in a huge commit. I would suggest to reformat the code progressively as it undergoes other modifications.

It might be worth enabling auto-formatting on save or on commit to ensure the usage of the agreed style.

[IBNobody]

@DidierLoiseau If you can suggest a method to autoformat between K&R and Allman on submit, I'd be onboard.

[jasonm23]

https://www.gnu.org/software/indent/manual/indent.html

[IBNobody]

Is there a way to integrate that into the git checkin process, though?

[jasonm23]

Use Git hooks.

We could add a script say, ./bin/setup-autofornatter which installs the dependencies and adds a pre-commit hook to run it on the changed files.

It's best to allow some sort of override, just incase it causes someone an issue.

Also our preferred usage will need documenting of course.

Cross platform issues will probably be the biggest issue, but at least it would help people stay standardised.

We can also recommend using editor plugins / formatters which also enforce K&R.

[IBNobody]

Will it easily unKR the code too? I would be willing to try something, provided it works with the Windows toolchain.

[jasonm23]

I recommend reading the docs and trying the tool standalone to see how it works.

Specifically here, it outlines that styles are an basically a collection of rules.
https://www.gnu.org/software/indent/manual/indent.html#SEC4

[DidierLoiseau]

A commit hook is indeed a solution, but TBH I would consider it as a last resort solution. I don't really like the idea that you can't see what will be committed, and that it could be difficult to restore if it gets messed up.

In fact, I am using Eclipse to work on QMK, and it is able to format code automatically on save if desired. I hadn't enabled this feature and now I remember why: it completely messes up the keymaps! And unfortunately, it does not support to turn it off through comments (feature request, vote for it!).

Another option would be to configure tools to analyse the code and output warnings when it does not respect the style. Some of these tools can do static analysis and detect potential bugs too. Such a tool could probably be configured to run as part of the build that runs on merge requests for example.

[jasonm23]

I'm also averse to this.

But, we don't have to configure indent (or any other tool) indiscriminately. Keymap files (and anything else we choose) can be excluded.

[jasonm23]

We can also lint / style check and provide warnings instead.

I think it's simple enough to exclude keymaps programmatically. Even if indent doesn't support comment matching, we can easily achieve the same result.

[skullydazed]

I'm going to close this issue for now. To summarize the situation with code style:

• Anyone looking for a guideline to follow should use OTBS
• Anyone modifying existing code should match the style of the code they're changing
• While we would prefer new files in core follow OTBS, we aren't going to turn down good code that follows another style.

[jceaser]

can we ask that methods be documented, it's really hard to figure out what is going on if your new here

[skullydazed]

@jceaser We have some efforts underway to help people in your position. We could use more and better comments, and we have also been working to improve our documentation. One piece of that that may help you is a rough draft of one of those documents:

https://github.com/qmk/qmk_firmware/blob/docs/docs/understanding_qmk.md