Nodemcu-firmware: Consistent indent style

[marcelstoer]

• agree on an indent style
• document it in CONTRIBUTING.md
• run the code formatter (which one?) once over the entire code base?

I suggest to also persist the style in an http://editorconfig.org/ file. That's likely the most sensible approach for a group without a common editor/IDE.

[TerryE]

I tend to K&R but following the Lua convention of two spaces for a logical indent. Tabs are just too much of a bugger on different visualisation systems to be worth the saving for a 2 tab indent. I not that the K&R convention of not indenting a single statement is quite common, and prefer 1TBS as this I've been burnt by this sort of error far too often:

  if (x < 0)
    puts("Negative");
    negative(x);
  nextStatement();  

as opposed to

if (x < 0) {
  puts("Negative");
  negative(x);
}

easy to miss, eh? But as to style, the vast bulk of of our source is the Lua core and this is written to a very consistent style, so my view is: _if we are going to encourage a unified style (which I believe that we should), then we should follow this Lua core style for all our C code._

As I have already started in #1165 and #1166, I think that the code base would be more maintainable if we did this sort of clean-up. I would suggest that we don't reformat imported libraries such as lwip et al, but as to the app/modules and any future additions, then yes definitely you have my :+1:.

[jmattsson]

Honestly, as long as the source is easily readable, I'm not fussed about 2 vs 4 vs TAB. We have more important things to work on given our limited resources, imo.

I do quite strongly prefer leaving braces off for single-statement if conditionals as it yields cleaner code. Of course, I tend to have a blank line after such a construct...

[TerryE]

My personal preference for one liners is to keep them on the same line as the if, while or whatever

 if (!p) return MEM_ERR;

But this isn't popular. In general, I think that most modules are reasonably consistent, but we could do this one on a lazy basis - that is leave it to the maintainer to decide. My main suggestion is that any code tidy should be done as a separate commit from any functional changes.

[pjsg]

The great thing about coding styles is that there are so many strongly held opinions. We went through this at work some time ago (for Java). In the end we compromised and nobody was 100% happy. We decided that the only way to not show favouritism was to adopt someone else's style guide _without_ _any_ changes.

I'm not proposing that we do this here.... Well, we could adopt a style, and then not enforce it!

My take on the examples above are 4 space indent, never use tabs, always use braces. e.g.

if (!p) {
    return MEM_ERR;
}

There was a famous security bug which was the result of missing braces after an if. The code read: (although the warning comment was not present!)

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;  /* MISTAKE! THIS LINE SHOULD NOT BE HERE */
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

This meant that a piece of the SSL certificate verification was not performed......

[devsaurus]

The great thing about coding styles is that there are so many strongly held opinions.

I agree. Although, if we don't give any requirement/recommendation/wish then we'll end up with a mixture of all of them. A contributor willing to adhere to existing coding style is not able to derive any consistent guideline at the moment IMO. Thus I'd be happy with any consolidated statement in CONTRIBUTING.md ... without strict enforcement.

Technically, I'm ok with the Lua style. It seems to boil down to

• 2 spaces indent
• braces acc. to 1TBS

"If unsure, use Lua style for new files. When modifying existing files, adopt to their coding style."

[marcelstoer]

I would suggest that we don't reformat imported libraries such as lwip et al, but as to the app/modules and any future additions

Yes Terry, that was my intention. Sorry I didn't state that explicitly.

• 2 spaces indent
• braces acc. to 1TBS

:+1:

"If unsure, use Lua style for new files. When modifying existing files, adopt to their coding style."

:+1: except that "...existing files, adopt to their coding style" shouldn't be necessary if we reformat everything in app/modules (and lua_examples & lua_modules) once we have an agreement.

[karrots]

+1 for all of this and running the existing files through something to make them all consistent.

I was going to make a proposal to run the files through something like WTF as it bothered me when my editor removed extraneous white space when editing files. Made for messy PRs.

[devyte]

Hi, from personal project experience, I can say that just defining the style to use will only mitigate the problem somewhat, but it won't really fix it, and will therefore still make for ruffled feathers. I suggest forcing use of a tool before a PR so that the desired style is always assured, e.g.: use astyle.
Either distribute the tool with the source, and call it before compiling the code (i.e.: make calls the tool as part of the default rule), or call the tool in some other way during a PR (maybe a hook?)

[marcelstoer]

I suggest forcing use of a tool before a PR

I'm interested what my fellow committers think of this but personally I'd give this a :-1:. As with (not) _enforcing_ certain Git/GitHub styles I wouldn't want to be policing too much, neither manually nor automatically, when it comes to code style. There certainly is value in consistency but we shouldn't overdue it. I'd like this to be a friendly, open and welcoming community. If some piece of code in a PR really is totally out of line we can still tell the author and hold back until fixed or reject it.

Should I be the one who eventually creates the PR I'd also propose adding the following .editorconfig as suggested earlier:

root = true

[*]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true

This is simple and very unintrusive as it won't affect you unless you use the EditorConfig plugin in your favorite code editor.

[jmattsson]

I suggest forcing use of a tool before a PR so that the desired style is always assured

Awwww heeeeeck no! That would be far too cumbersome. We want people to actually contribute...

As someone who contributes most things on company time, that would be enough to make me not bother and just leave things in the company repo instead. We have our own coding styles at work, and it's pretty much a given that whatever this project picks (if insisting on a single thing), it would be incompatible. Wasting time reformatting source is not something I can readily justify.

I thought people had gotten over the whole one-true-coding-style-to-rule-them-all, but it seems I was mistaken. Seriously, endeavour to write clean, readable, working code. Whether it's 2, 3, 4 or 27 spaces indent, if it's clean, readable and well structured, that's enough (though with 27 spaces that would be a challenge I suspect). Everyone has preferences, and the reasons aren't just emotional. A colleague of mine is slightly dyslexic, yet (or because thereof) strongly prefers to omit whitespace around operators and only use it around logical groupings. For me, I find white-spaced operators easier to scan and parse, but that doesn't mean either of us forces their style onto the other.

@marcelstoer the editorconfig would seem an appropriate level of intrusiveness, imo.

[TerryE]

I think that what this debate boils down to is:

• When we do get around to writing a guideline for module writers, then include an advisory style guide of " follow the Lua style: 2 space indent + 1TBS + no trailing spaces".
• If any contributor is doing non-trivial rework to a module that is inconsistently laid out and not following this, then at their discretion they can reformat the module so long as the reformatting is done as a separate commit from the functional changes.

In this way we can converge towards a coherent style for modules without being proscriptive. Anyway that's my conclusion.

[devyte]

Wow this sounds like a whole lot of misunderstanding...

@marcelstoer My suggestion is not to police the style, meaning I'm not proposing a style checker, which would fail when the style doesn't comply. I'm proposing an automatic reformatter. The contributor codes up his change, builds the code, and it automatically complies with whatever style you guys defined.
The thing about a not-enforced guideline is that most people won't stick to it, or will stick to it only marginally. Then, sooner or later a contribution will be rejected as a result. That just wastes time and effort both for the contributor and the reviewer! You've already gone through something similar with documentation: you had a guideline, people didn't stick to it, the documentation was a mess, and you had to automate it.

@jmattsson I'm a bit like you, I'm working on this on company time half the time. I don't understand why you think this would require _more_ time on the contributor's side? I would argue the opposite: it _saves_ time, because as a contributor you no longer need to worry about style: it gets done automatically. You can startup your editor X, and not have to worry about whether its setup complies. Also, given that a style has been agreed on for the guideline, there is no argument about what style should be used.

In my previous job (I did software R&D for ESO Paranal, the biggest optical telescope in the world), the whole style thing was a big topic. It caused a lot of aggravation, and I witnessed more than a few yelling matches between colleagues. In my current job (I do hardcore software R&D for Synopsys, the biggest EDA company lol) this _used_ to be a topic that caused aggravation, until management decided to enforce style with an automatic tool like I suggested here. Its use is transparent: any code written gets reformatted on submitting to the repo, so we as developers don't have to worry about it, or spend even a single minute messing around with whitespace or style. There are at least 5 different setups for editors and such that I'm aware of among colleagues (vi/gvim like I use, emacs, eclipse, an in-house IDE called IDE@S, and KDevelop), and style is always seamless and consistent now. Exactly zero wasted effort! And when I'm opening a source file, I know exactly what style to expect, no surprises :)

I'm hoping to contribute what I've seen has worked well elsewhere wrt the development process, but if you still wish to go a different way, ok, you're the main contributors here, so it's your call :)

[marcelstoer]

Thanks for the feedback. Not sure I got all of it, though.

The contributor codes up his change, builds the code, and it automatically complies with whatever style you guys defined.

And then? If the contributor updates the personal (forked) repo from upstream she still gets the reformatted code. She should then re-format again with the personal settings? I've never worked under such a regime but it doesn't sound like something I'd be happy with.

The thing about a not-enforced guideline is that most people won't stick to it, or will stick to it only marginally.

Well, I don't think we _want_ to enforce a code style.

You've already gone through something similar with documentation: you had a guideline, people didn't stick to it, the documentation was a mess, and you had to automate it.

Uhm...no. It used to be a wiki without guideline and zero QA. No wonder it was a mess. Even after the clean up it's still 100% manual labor (except for the transition to HTML at RTD). Every change is reviewed because the only way non-committers can change the docs is through PRs. The biggest contributor to better docs was moving them from the wiki to the repository. It's no longer "out of sight, out of mind."

[devyte]

@marcelstoer You implement your code, you build the code, your changes get automatically reformatted as part of the build process (this is one option, the other is they get reformatted as part of the PR or merge), and so the changes comply with the chosen style. Only your changes could be reformatted, not the entire code base. The code is then up for review, everyone who reviews it is looking at code with the expected style, so no eye sores. The changes get merged into the repo. Done.
Next developer pulls, gets code that is at least partially formatted with the chose style, so no eye sores. He implements changes, builds the code, the changes get automatically reformatted, and you do the PR. Code gets reviewed, and so on...

git offers hooks as an integral part of its features, which should allow reformatting on push/merge/whatever, if you don't want to do it on build. I've never used them, I use perforce at work, and simple git for personal uses, but it shouldn't be too hard to figure out.

If somebody wants to work with personal settings that are different, it's also possible. In fact, we had originally thought of doing this at work: pull the code from the repo, reformat to your own preferences, then you dig in. Once done, the changes get reformatted to the common chosen style, and submit to the repo. However, the idea was dropped, because everyone decided to simply get used to the chosen style.
If desired, it should be possible to do in a similar way as how the project files are being discussed in #1177: each developer keeps a personal setting locally, which is used to reformat on pull, or reformat manually, or whatever, before starting work. This would allow everyone to work in their own personal style, but still comply with a common style for the developer group as whole.

My personal preference for C/C++ style is Allman (aka BSD), but then again I'm a fossil. In general, I prefer to work directly with whatever style is defined by the developer group, so I wouldn't reformat back and forth. In this case it sounds like everyone wants a K&R variant.

What @pjsg said about the security bug due to bad indentation is something I've seen too many times. It even happened to a very experienced colleague, and it caused quite a bit of embarrassment, because the bug ended up affecting a big customer. I haven't seen it happen since the automation, not even to new interns who are still green behind the ears. I was against the whole automation thing at first, but overall it has been a very positive thing, which is why I feel strongly about it.

Anyways, please let me know if you'd like to discuss further, or if I should just shut up about it :)

[marcelstoer]

So, let's wrap this up. I'll prepare a PR:

• add a section to CONTRIBUTING.md for now as "When we do get around to writing a guideline for module writers..." is in some distant future
• mention " follow the Lua style: 2 space indent + 1TBS + no trailing spaces" and
• back this up with the EditorConfig listed earlier

How about one-off auto reformatting? Terry, in your first comment you stated

I would suggest that we don't reformat imported libraries such as lwip et al, but as to the app/modules and any future additions, then yes definitely you have my :+1:.

which I interpreted as "yes, do reformat" but in your last comment you said

... then at their discretion they can reformat the module...

which I take as a no. Can we have a vote among those committers who have left comments yet? Just edit this comment and put a thumb (up or down) next to your name.
myself :+1:
Terry :+1:
Johny
Arnim :+1:
Philip

[TerryE]

@marcelstoer I am getting old and my daughter is getting married on Sunday. I am not consistent but I am contented. :smile: I would prefer to do a clean sweep, but this isn't going to be a 100% automated exercise and who is going to step up to do the manual follow through? On reflection, I realise that doing this one module at a time when they really need doing is more preferable than us agonizing over this, so I've added my +1. One step forward. :)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[TerryE]

I note that the Lua VM code includes non-braced indents on if statement. Our own modules have a random medley and I doubt that anyone has the energy to do a bulk reformat, so after 2 stale cycles I am closing this. Any committers who disagree and _want to do this clean up_ please reopen this issue.