Clean up & extend Lua code style guidelines

[appgurueu]

Separated into the clean up and the extension.

Requires input from @minetest/engine.

Motivated by the recent pour-in of Lua PRs (mostly related to the main menu).

[mark-wiemer]

LGTM as a noob, left some minor cleanup comments :)

[mark-wiemer]

These two bullets are a bit confusing to me as a noob, is it ever OK to just have function name(...) or must it always be local function name(...)?

Suggested change

	* Always use `function name(...)` instead of `name = function(...)`.
	* Use `local function name(...)` instead of `local name = function(...)`,
	irrespective of whether the function is recursive or not.
	* Always use `local function name(...)` instead of `local name = function(...)` or `name = function(...)`.

[rollerozxa]

Global functions do still exist somewhat in builtin iirc, but local is to be preferred unless it's necessary. Same syntax is used for defining functions inside of a table though, without a local in the function definition when the table itself is local.

But in this case it just means that you shouldn't write it like you're assigning functions to a variable JavaScript-style.

[mark-wiemer]

Understood, I just hope we can update the PR to reflect the same. As it stands, I believe noobs like me will be a bit confused as to when/if to use function name vs local function name. I know it's clarified later, but that's way later as the PR currently stands.

[rollerozxa]

Looks good (with some of Mark's suggestions). Leaving it to another coredev to approve before it is merged.

[Desour]

I think we should have simpler, shorter, and less strict guidelines, and made my comments accordingly.

Think of this: If someone offends a rule, do we want to wait for them to fix it before merging? If this part of the style is inconsistent in our code, do we care? Does it make it hard to read?

[Desour]

Isn't both fine?

I personally don't care which is used and if they're mixed.
And I don't think we should be that nitpicky.

[appgurueu]

There is a tangible downside to using string.* functions: They silently coerce numbers to strings. This has previously caused problems, because then you get functions which silently accept numbers instead of strings and when you change them to use method-style you get crashes. Hence I prefer new code be written more strictly, in line with the "don't rely on coercion" rule I've added.

I can add a footnote including this reasoning if you want.

Also, I care, and prefer them not to be mixed. To give an extreme example: s:match("foobar(.*)"):lower():trim() is much more readable than string.trim(string.lower(string.match(s, "foobar(.*)"))). (And to give a deliberately atrocious example of mixing for the fun of it: string.lower(string.match(s, "foobar(.*)")):trim() which has to be read as middle - left - right.)

Lua is not the only language that lets you choose between imperative and method style for such methods. In Python you could for example do str.find("a", "b"), but I haven't seen anyone do that, ever. The only thing these namespaces are useful for is in some functional code so you can reference str.find without an instance at hand, but not for direct method calls.

[Desour]

They silently coerce numbers to strings.

Oh, I didn't know they did.
But relying on method style call to error is also kinda ugly, and won't produce easily understandable error messages. 🤷

Also, I care, and prefer them not to be mixed. To give an extreme example: s:match("foobar(.*)"):lower():trim() is much more readable than string.trim(string.lower(string.match(s, "foobar(.*)"))). (And to give a deliberately atrocious example of mixing for the fun of it: string.lower(string.match(s, "foobar(.*)")):trim() which has to be read as middle - left - right.)

Yeah, in these examples of nested calls, method style syntax is easier to read.
But I was more thinking of something like: string.match(s, "foobar(.*)"):lower():trim().
Or compare ("foo%s"):format(a) vs. string.format("foo%s", a).

[appgurueu]

I will usually write ("foo%s"):format(a), but we can add an exception for string.format because the string is often a constant there and in contrast to e.g. Python Lua requires you to parenthesize that, which would be a syntactical oddity for many.

[SmallJoker]

If the guidelines mention using tabs for indents, then why are the code examples not using tabs?

[appgurueu]

If the guidelines mention using tabs for indents, then why are the code examples not using tabs?

Hehe. I wondered whether I should bring up this detail. I'm using Markdown indentation (2 spaces) here. In principle I should be able to indent using tabs everywhere, though then the list item bodies won't line up as nicely. Ultimately this problem stems from Markdown indentation vs. code indentation. I could mix the two I suppose, but that might end up a little cursed to edit.

Related: #76. I suppose indentation is something we do need to standardize rather sooner than later.

[mark-wiemer]

I'm not too concerned with indentation, simply because engine code contributors will see how indentation works in example files nearly instantly. And for this site, we can standardize it with an automated tool (Prettier) and do a quick automated fix of the whole repo at any time--I've done it plenty of times before :)

[appgurueu]

we should have simpler, shorter [...] guidelines

Even after these extensions, the guideline (which previously was primarily concerned with whitespace it seems) only has ~1888 words, meaning it can be read in ~8 minutes and read aloud in ~10 minutes. I think that is, as far as length of a code style guideline goes, definitely acceptable. (Ad populum: You might want to compare e.g. to the guidelines we reference or other code style guidelines you find.)

Many of the rules boil down to "use what little (syntactic) sugar Lua has to offer except for what is commonly considered weird (e.g. omitting parentheses for function calls ¹)"

[I think we should have less strict guidelines.] If someone offends a rule, do we want to wait for them to fix it before merging? If this part of the style is inconsistent in our code, do we care? Does it make it hard to read?

This style guide is normative / idealistic. It makes no statement about which extent it should be enforced to, how "strict" it is (in fact, the wording explicitly suggests to apply "best judgment", which includes not being unnecessarily pedantic in reviews). It ultimately degrades to a bunch of "SHOULD"s, with some "SHOULDs" being stronger than others. We can try telling people what they should do, and we can reasonably turn a blind eye if they don't and we don't mind too much.

I'm well aware that it won't magically transform our code, contributors or core developers, and that our code will never 100% follow it (and we should not strive for our code to 100% follow it because we'd get diminishing returns); the old style guide has already been applied inconsistently.

But especially new contributors who read a style guide, maybe their first Lua style guide ever, probably won't have strong opinions or habits concerning most of these style questions, which we should take advantage of by influencing them to write code in a consistent way, nudging them towards what we consider good style.

(As for personal feelings, I am irritated by inconsistencies, and I find it easier to scan for some things when the code is written in a consistent way, and I do think that many of my suggestions here are just better practices than the alternatives people may choose.)

I agree that we want to reduce tedious manual work both on the end of contributors and reviewers. I would suggest automating at least much of the whitespace discussions away (which may require some changes to the whitespace-related parts of this guide) by adopting a formatter such as https://github.com/JohnnyMorganz/StyLua. We'd have to run it once on our codebase, cluttering our history, but after that we could use it to check formatting in CI and to format files automatically.

Footnotes

1. personally I like to do this, but others don't like to see it ↩

[SmallJoker]

because engine code contributors will see how indentation works in example files nearly instantly.

@mark-wiemer Providing wrong examples especially in the code style guide gives it a bad touch.

I should be able to indent using tabs everywhere, though then the list item bodies won't line up as nicely.

@appgurueu Breaking a bullet list for a few lines of example code is not a big deal. The overall structure of the document is still visible. I priorize technical accuracy, thus I'd personally sacrifice some of the visual fidelity.

[mark-wiemer]

@SmallJoker providing wrong examples is certainly not ideal, no. But it looks like the PR is 90% good and 10% bad indentation--I'm happy to merge the current PR and we can figure out a follow-up indentation PR at this point. Don't want perfect to be the enemy of the good :)

[appgurueu]

No big deal either way, I've just converted indentation across the whole file to tabs for now which works just fine.

[SmallJoker]

LGTM 👍