Naming of boolean setters/getters

[akien-mga]

We have a lot of inconsistency in the current codebase on how we name boolean setters and getters, and especially those we expose to the scripting API.

We should aim at fixing that for 4.0, breaking compatibility where needed. Related to #16863.

Current uses are of the form:

bool stuff = false;

void set_stuff(p_enable);
void set_stuff(p_enabled);
void set_stuff_enabled(p_enable);
void set_stuff_enabled(p_enabled);
void set_stuff_enabled(p_stuff);

bool get_stuff();
bool get_stuff_enabled();
bool is_stuff();
bool is_stuff_enabled();

Can also be more tricky if the boolean name is verb, like:

bool process = false;

void set_process(p_enabled);
void set_processing(p_enabled);

bool get_process();
bool get_processing();
bool get_process_enabled();
bool get_processing_enabled();
bool is_process();
bool is_processing();
bool is_process_enabled();
bool is_processing_enabled();

I'm not sure what style we should settle on, but it needs to be consistent.

Here's a proposal to start the discussion:

• Name boolean themselves as nouns, e.g. processing instead of process, which solves some of the issues shown above.
• Use p_enabled as parameter in the setter.
• Use set_<boolean> for the setter, and is_<boolean>_enabled as getter.

Example:

bool processing = false;
void set_processing(p_enabled);
bool is_processing_enabled();

[aaronfranke]

Just my two cents, but I prefer properties to getters and setters. I would prefer to have

bool processing = false;

Then you can have:

if processing:
    processing = false

As opposed to:

if get_processing():
    set_processing(false)

If we're changing a bunch of getters and setters, we could just remove them.

[akien-mga]

Properties are defined via a setter and a getter, so we still need to name them...

[rxlecky]

Related to what @aaronfranke mentioned, but in C++ context rather than GDScript:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c131-avoid-trivial-getters-and-setters

Any plans of getting rid of trivial getters/setters?

[pouleyKetchoupp]

@rxlecky This guideline doesn't explain what are the advantages. Using public members instead of trivial getters/setters can be a problem when one of them becomes non-trivial for any reason, and you end up modifying your whole codebase rather than one method. So I would argue against doing it systematically, even if in some cases it makes sense to improve code readability (like around math stuff, which is already the case in Godot).

[sankarsh98]

@pouleyKetchoupp You make a really valid point here Using public members instead of trivial getters/setters can be a problem when one of them becomes non-trivial for any reason, and you end up modifying your whole codebase rather than one method.

[aaronfranke]

PathFollow2D has a rotate bool, along with set_rotate and is_rotating. This is the same name as Node2D's rotate method, and this causes warnings in the Mono module... perhaps this bool should be renamed? Perhaps rotating, with set_rotating and is_rotating/get_rotating, or rotates with set_rotates and get_rotates/can_rotate/is_rotating? Also relevant for #16863.

[Calinou]

It would be interesting to make a script to extract boolean getters/setters from the codebase and rename them to follow the convention outlined in the proposal. This way, we can quickly see if any of them look particularly awkward.

[madmiraal]

See proposal godotengine/godot-proposals#1994.

[akien-mga]

Superseded by godotengine/godot-proposals#1994.