Lua code style

[vakhoir]

It seems we don't have a consistent coding style for Lua. I should have asked about this from the start, but I figured I'd just copy the style from files that were already committed. The problem is that the PiGui style is quite different from the old UI, and module scripts. The old UI style seems to be closer to the C++ one, and makes more sense to me. The biggest discrepancies seem to be:

Code: Select all

import 'path/to/file.lua' -- pigui style
-- vs.
import(path/to/file.lua) -- old ui stlye

Code: Select all

object:methodName() -- pigui style
-- vs.
object:MethodName() -- old ui stlye

Other rules that seem to be applied consistently in the old UI:

• every variable that gets set through an "import" starts with upper case
• every other variable starts with lower case

I'm not going to drop everything to refactor the code, but I think it would be a good idea to pick a style going forward, and to update the wiki.

[sturnclaw]

Ah, this one is a story the lua maintainers wouldn't tell you...

I kid, I kid.

This is partially my fault, because I'm used to writing code in the Lua style (e.g. setmetatable), with a dash of snake_case mixed in. It seems the original Lua authors (robn and a few others) were a fan of PascalCase, something which I cannot fault them for, and ecraven (who by and large wrote all the pigui code) used camelCase, otherwise known as Javascript style.

Interestingly enough, all of the code prior to when I joined the project used the function-call style for importing; the newer string-literal call style that occasionally shows up is entirely my personal style. I like it and am probably going to standardize it because it makes it extremely easy to see what you're importing, it closely follows Lua norms, and it effectively communicates that you're bringing in a module name to the local scope.

Code: Select all

import("a/b/c.lua") -- the existing function call style

require 'module.name' -- the new, "elegant" style

Opinions vary on consistent style; at the moment you are the primary Lua contributor so your voice has the most weight, but as the de-facto Lua maintainer my recommendation and personal style (I'll get it on the wiki eventually, I swear!) is as below:

Code: Select all

-- use the string-literal method with require() instead of import, due to import being on its way out the door
-- don't use filesystem paths, use Lua's module.name syntax; paths are also being deprecated now
-- also worth noting is to please name your files after their primary class, maintaining the same case
local MyNewClass = require 'lib.MyNewClass'

-- local / static variables at file or function scope should use snake_case
local my_size_var = ui.rescaleUI({ ... })

-- Types / Classes should use PascalCase, as in Engine or PiGui.Modules.Face
local class_obj = MyNewClass()

-- the pigui object is imported as lowercase because it's a reference to a dispatch object and not a type
local ui = require 'pigui'

-- instance variables should use camelCase for naming, to distinguish them from local variables and instance methods
class_obj.instanceVariable = my_size_var

-- class and instance methods should ideally use PascalCase as well, although I'm not opposed to using camelCase if necessary
class_obj:DoSomethingWithInstanceVariable()

-- methods bound to C++ should use PascalCase, with no exceptions
class_obj:CallSomeCPPMethod

However, nothing is perfect. We have enough code in pigui that uses camelCase instead of PascalCase that it's not worth refactoring everything - my rationale behind this is that this indicates that it's a wrapper over the C++ PiGui methods, although that's entirely an inconsistent back-explanation instead of a style choice. I occasionally slip into C++ style camelCase for local variable names, and pigui is universally imported as lowercase 'ui' because it's a lot easier to write like that, and it's also not a class, rather being a collection of function dispatchers.

Going forward, I'd recommend that everything but UI code uses this style guide; I'm willing to make a stylistic difference for function and method names in the UI code just because that's about 3,000 instances that would need re-formatting and we don't have a good tool to do that automatically.

If you have suggestions I'd love to hear them; this style guide is mostly my personal style matched with existing code in the project (with some exceptions thrown in for the UI code). I'd like to think that most of these are fairly common-sense and don't cause confusion / hair loss when writing / maintaining Lua code, but I'm more than happy to take feedback on board.

...reading what I've written, it boils down to "use camelCase for UI code, use PascalCase for game code". Everything else is as outlined in the style guide above.

[vakhoir]

Code: Select all

-- use the string-literal method with require() instead of import, due to import being on its way out the door
-- don't use filesystem paths, use Lua's module.name syntax; paths are also being deprecated now
-- also worth noting is to please name your files after their primary class, maintaining the same case
local MyNewClass = require 'lib.MyNewClass'

-- local / static variables at file or function scope should use snake_case
local my_size_var = ui.rescaleUI({ ... })

-- Types / Classes should use PascalCase, as in Engine or PiGui.Modules.Face
local class_obj = MyNewClass()

-- the pigui object is imported as lowercase because it's a reference to a dispatch object and not a type
local ui = require 'pigui'

-- instance variables should use camelCase for naming, to distinguish them from local variables and instance methods
class_obj.instanceVariable = my_size_var

-- class and instance methods should ideally use PascalCase as well, although I'm not opposed to using camelCase if necessary
class_obj:DoSomethingWithInstanceVariable()

-- methods bound to C++ should use PascalCase, with no exceptions
class_obj:CallSomeCPPMethod

This mirrors my preferences perfectly!

...reading what I've written, it boils down to "use camelCase for UI code, use PascalCase for game code". Everything else is as outlined in the style guide above.

What do you think about codifying the rules you outlined above, and just accepting the ui code as wrong at the moment (we can even add a comment at the top of ui files to that effect)? Every new function should be PascalCase, and we'll get to refactoring it at some point. I think biting the bullet on a mistake that was made makes more sense than carving out an exception for a section of the code.

P.S:

There's also the question of filenames. Just noticed we're using CamelCase in most code, and lowercase with dash seperators in pigui

[sturnclaw]

What do you think about codifying the rules you outlined above, and just accepting the ui code as wrong at the moment (we can even add a comment at the top of ui files to that effect)? Every new function should be PascalCase, and we'll get to refactoring it at some point. I think biting the bullet on a mistake that was made makes more sense than carving out an exception for a section of the code.

P.S:

There's also the question of filenames. Just noticed we're using CamelCase in most code, and lowercase with dash seperators in pigui

I'd like to preface this by saying I've had my head in C++ for too long. On a read of some of our Lua code, it seems I'm slightly off base here; we seem to have standardized on camelCase for file-scope variable names instead of snake_case. I don't have a strong preference for either, so I'd rather err on the side of the one that takes less refactoring to adopt.

This is going to sound possibly contradictory, but I don't think we actually need to change much to bring the pigui code into compliance. You're already using PascalCase for method names on the classes you're writing; we use camelCase on instance variables (including delegate / handler functions), and like I mentioned before the 'ui' object is not a class or an instance, but rather a dispatch object, so its functions should not be PascalCase. About the only things that need to be changed are the import paths and the casing on some of the mixin objects I've written.

I'd like to clarify a little - PascalCase is only for class or instance methods (or C++ methods). If it's PascalCase, you should be self-calling. If it's not, don't use PascalCase. If you write a prototype or dispatch object, its members should be camelCase. Because the UI code is so different to our "game code" (we avoid OO in the majority, we make heavy use of immediate code, raw functions, and prototypes instead of classes) this distinction aligns almost perfectly with the existing code style of both the UI and the game code.

Here's a clarification on some of the sticking points of the style guide:

Code: Select all

-- this is fine; the less refactoring we need to do the better
local fileScopeVariable = Vector2()

-- local-scope functions should use the same naming convention as local variables
local function drawSomeShipData()
	-- ...
end

local ui = require 'pigui'

-- PascalCase is correct, we're creating a new type / class here
local MyWin = {}
MyWin.__index = MyWin

function MyWin.New()
	return setmetatable({
		-- this is correct; handlers / dispatchers are instance variables not methods
		someDispatchHandler = function() end
	}, MyWin)
end

-- This is an instance method, so PascalCase is correct.
function MyWin:CallHandler(...)
	-- the ui object is not a class or an instance, and we're using regular dot-call notation,
	-- so the functions it contains should not be PascalCase.
	ui.doSomeThing()
	return self.someDispatchHandler(...)
end

-- We're exporting a class here, so the file should have the class name.
return MyWin

Regarding filenames... this is definitely a hill to die on. I'd say if the file exports a class, it should use PascalCase. If the file does not, it is at your discretion on how to name it.

This post has gone through about five revisions in an attempt to settle on a common-ground style, so if something seems confusing or not well explained, please feel free to question it thoroughly!

[sturnclaw]

With the revisions above, I'm good to standardize this and start applying it to new files (and refactoring as necessary). I'll update/write a new post on the wiki containing the final style guide, and take whatever feedback you have into account as I do.

[vakhoir]

This is going to sound possibly contradictory, but I don't think we actually need to change much to bring the pigui code into compliance. You're already using PascalCase for method names on the classes you're writing;

Well... I've been somewhat inconsistent, sometimes it's camelCase, sometimes it's PascalCase, and I'm pretty sure I've run afoul of the Type/dispatch object distinction. Speaking of which, I feel a bit iffy about it as it doesn't seem too intuitive, but I'd go forward with it anyway, since once it's defined it is a clear rule anyone should be able to follow. I'd just make sure we have it in the documentation, and perhaps we can add a comment block in pigui.lua explaining the whole idea.

With the revisions above, I'm good to standardize this and start applying it to new files (and refactoring as necessary). I'll update/write a new post on the wiki containing the final style guide, and take whatever feedback you have into account as I do.

Yeah, let's do this!

[impaktor]

Please update wiki with whatever you've agreed upon, and probably do that before staring applying it to the code base:
https://pioneerwiki.com/wiki/Code_style

Possibly, that entire page can be deleted/re-written to two short sections: C++ and Lua
1. C++ follows scripts/clang-format.sh on staged files to-be committed
2. Lua as you've agreed upon above

The less text the better, if it's exhaustive