Coding Style Guide - efroemling/ballistica GitHub Wiki

Motivation

One principle in the Ballistica project as compared to the original BombSquad code is more organized code and coding conventions. When I started writing BombSquad I was not very concerned with such things since it was just me, but now my hope is to get more people involved, and in my experience it is good to enforce common standards in group projects to keep things consistent and equally accessible to all. Some of this can be enforced automatically via linters, but some more stylistic and philosophical choices cannot be. This page will serve as a list of those. If you plan to contribute to the project, please look over these and try to be a 'good citizen' by conforming as much as possible to these conventions. And if you feel that something should be added or changed here, let's discuss it.

Lint Checks

The root project Makefile contains several targets used to check code. You can run make check to run them all, or run make help and look under the 'Checking' section for individual ones, which can be more efficient. For instance, make pylint will run only PyLint checks, which encompasses most style-related checks. Note that in order for code to be accepted into the project it will need to pass all of these tests. (if make preflight passes, you can be pretty certain your code is acceptable).

Python Guidelines

Base Guidelines

Follow all rules in PEP-8, Python's official style guide. This covers many areas such as capitalization, spacing, and naming conventions.
In addition, follow the Google Python Style Guide when possible (minus the exceptions listed below). This includes many smart coding practices which help keep things readable and maintainable by others.

Exceptions, Clarifications, and Additions:

Auto-formatting is your friend.

Our auto-formatting will handle a large amount of PEP-8 compliance. Run make format in a terminal in the root of the project to auto-format everything. This will take a little longer the first time you run it but should be very fast afterwards. Remember to reload your files after you do this if your editor doesn't do so automatically. It can sometimes be convenient to code 'quick and dirty' and rely on this auto-formatting to make things pretty afterwards.
Limit lines to 79 characters.

As per PEP-8. This should generally be handled by auto-formatting, but sometimes you may be required to break up long strings/etc. I used to be against the idea of limiting line lengths based on arbitrary technical limitations of long-obsolete equipment (come on, what modern monitor can only display 80 lines!?!), but over time I've come to see it rather as an agreed-upon standard allowing multiple editors to be set up side by side while ensuring that all code is visible. And I think it subtly encourages some good coding practices such as avoiding excessive nested conditionals since that results in very cramped looking code.
Use spaces, not tabs, and 4 spaces per indentation level.
Avoid wildcard imports.

Don't do things like: from foo import * because it makes it unclear which names are present and where names come from.

Importing classes/functions is ok.

Section 2.2 of Google's style guide states to never import classes or functions directly, but this is allowed in Ballistica, as I feel it is often the cleanest looking option for working with the game's package hierarchies.

# Too wordy (though maybe ok for limited use):
import bastd.ui.config
checkbox = bastd.ui.config.ConfigCheckBox()

# Less wordy, but 'config' is not very descriptive by itself:
from bastd.ui import config
checkbox = config.ConfigCheckBox()

# More descriptive but adds extra names to keep track of:
import bastd.ui.config as stdconfigui
checkbox = stdconfigui.ConfigCheckBox()

# Feels cleanest as long as the class name is sufficiently descriptive:
from bastd.ui.config import ConfigCheckBox
checkbox = ConfigCheckBox()

Use descriptive public names.

In line with the previous note, try to keep public names significantly descriptive on their own without relying on the package hierarchy to clarify their meaning, even if this results in a bit of redundancy. For instance, the class name 'bastd.ui.appinvite.AppInviteWindow' is preferable to 'bastd.ui.appinvite.Window'. This way the class can be imported directly without resulting in an ambiguous 'Window' name. This also enables a design pattern where all public names are imported into the top level package module and the package hierarchy is essentially just for internal organization (see the 'ba' package as an example of this).
It's ok to use staticmethod.

Section 2.17 of Google's style guide states to avoid @staticmethod and instead use module-level functions for such purposes. Because Ballistica encourages importing classes directly, @staticmethod is acceptable too as it can keep functionality bundled with its relevant classes and avoid the need for additional imports.
Don't explicitly inherit from object.

Section 3.9 of Google's style guide says to always have classes inherit from 'object' if they do not inherit from other classes. Since Ballistica is Python 3+ only, this is unnecessary and should be avoided.
Use f-strings.

Section 3.10 of Google's style guide says to use the format method or % operator for formatting strings. Since Ballistica is using newer versions of Python, f-strings are available and preferred, as they are generally more efficient and more readable.

Avoid backslashes for line continuation.

In many cases parentheses can be used instead, which will allow auto-formatting to work better.

# Don't do this:
# (running 'make format' will ignore these lines due to the '\'s)

from typing import Any, Optional, Dict, Union, Iterable, ClassVar, \
     List, Set, Mapping, Tuple

my_val = first_value + second_value + third_value + fourth_value \
     + fifth_value + sixth_value + seventh_value + eighth_value

# Do this:
# (running 'make format' will keep these lines nice and pretty for you)

from typing import (Any, Optional, Dict, Union, Iterable, ClassVar,
                    List, Set, Mapping, Tuple)

my_val = (first_value + second_value + third_value + fourth_value
          + fifth_value + sixth_value + seventh_value + eighth_value)

Influence code layout with trailing commas.

Ballistica's auto-formatting is set up so trailing commas can be used to hint at code layout. Use your best judgement on what is more readable for your particular set of data, function call, etc. There are corner cases where our auto-formatting looks unreadable by default but by futzing with trailing commas it can be much improved. Remember, however, that in certain cases a trailing comma can change the meaning of code: (1) is simply an int within (unnecessary) parentheses while (1,) is a tuple containing a single int.
```
# Running 'make format' will give the following results
# without and with a trailing comma, respectively:

mylist = ['first', 'second', 'third', 'fourth']

mylist = [
    'first',
    'second',
    'third',
    'fourth',
]
```
Handle 'Existable' objects with care.

Some Ballistica classes such as ba.Node, ba.Player, ba.InputDevice, and ba.Actor represent entities that can cease to 'exist' at some point independent of their Python lifecycle (as revealed through their exists() function). An example of this is a player disconnecting from a game; although the corresponding ba.Player objects will remain for as long as references to them remain, attempting to use them after the player has left (when exists() returns False) may result in errors. The following practices should be observed to minimize the chance of such problems occurring:
1. Code that optionally accepts an 'Existable' object should use the value of None to represent the no-object case; it should never accept non-existing objects for that purpose. For example, the constructor for an explosion object which may or may not originate from a ba.Player could take a source_player argument with Optional[ba.Player] as its type. This clearly communicates to the caller (and to the static type checker) that a player is optional, which would not be the case if it only accepted only ba.Player and checked exists() at runtime.
2. Code receiving an 'Existable' object as a function argument/etc. should assume that the object still exists, and is encouraged to use statements such as assert obj.exists() or simply assert obj to verify this. If an invalid object is passed, it is the fault of the caller and should be fixed on that end either by passing None or by avoiding the call. The ba.existing() function can be useful on the calling end for handling such cases by converting invalid objects to None.
3. Code passing or otherwise exposing 'Existable' objects should be written defensively if there is any chance that the object has died since it was acquired. For instance, if a ba.Player reference is stored at the start of a game and then passed to a function at a later time, it should be passed as ba.existing(player), which will translate invalid references to None and inform the type system that it is now an ba.Player | None, not simply a ba.Player. If, however, the player reference gets used immediately at the start of the game when it is received, it can be logically assumed that it still exists and can be passed directly (player departures are high level events on the main event loop and it can be assumed they will not happen in the middle of a continuous block of code).
```
# Examples of handling 'Existable' objects properly:

def print_player_name(player: ba.Player) -> None:
    """Print a name given a player."""

    # We should assume the player we were passed still exists.
    # (the caller needs to be fixed otherwise)
    assert player.exists()

    print(f'Player name is {player.get_name()}')

def print_possible_player(player: ba.Player | None) -> None:
    """Print a player name if a player is passed."""

    # IF we were passed a player, we should assume they still exist.
    # (the caller needs to be fixed otherwise)
    assert player is None or player.exists()

    if player:
        print(f'Player name is {player.get_name()}')
    else:
        print('No player was given.')

def print_player_later(player: ba.Player) -> None:
    """Print a player name later if they are still around."""

    # We should assume the player we were passed still exists.
    # (the caller needs to be fixed otherwise)
    assert player.exists()

    # Print the player name one second in the future. However, they may have
    # left before our print fires, so we'll need to use ba.existing() to
    # convert an invalid reference into None (which changes its type from
    # 'Player' to 'Optional[Player]' in the type-checker's eyes).
    ba.timer(1.0, lambda: print_possible_player(ba.existing(player)))
```

Expose complete, strong types by default.

Ballistica's type checking makes use of Python Generics, which means that sometimes the type checker knows more about object types than Python itself does at runtime. For instance, if an object is declared with the type list[float] then the type checker will prevent us from writing code that adds a string or a bool to that list. At runtime, however, all lists are simply of type list and Python will happily let us add anything to it. So for type checking to be effective, we need to make sure that our complete static types remain intact in as much code as possible so we don't silently do illegal stuff.


# Example of unbound generics reducing the effectiveness of type checking:

mystrings: list[str] = ['foo', 'bar']
mystrings.append(1)  # Mypy will flag this as an error as we would expect.

def add_int_to_list(targetlist: list) -> None:
    # targetlist is an unbound generic, ie: list[Any], therefore
    # we can add anything to it without triggering errors.
    targetlist.append(1)

# Mypy will NOT flag this as an error although it breaks our internal logic. 
add_int_to_list(mystrings)

These same principles apply to Ballistica's generic types: things like ba.Activity, ba.Player, and ba.Team. For example, ba.Player declarations can specify a team type and ba.Team declarations can specify a player type, which allows code such as the following to be type-checked: (see notes here on using the reveal_type() function)


# Define a MyTeam that knows it contains MyPlayers and vice versa:

class MyTeam(ba.Team['MyPlayer']):
    pass

class MyPlayer(ba.Player[MyTeam]):
    def __init__(self) -> None:
        self.score = 0

# This function properly uses concrete bound types:
def print_teammate_scores(player: MyPlayer) -> None:
    for i, p in enumerate(player.team.players):
        print(f'teammate {i} score is {p.score}')

        # Because Mypy knows that player is a MyPlayer, it knows
        # that player.team is a MyTeam and thus that
        # player.team.players is a list of MyPlayers. Thus,
        # reveal_type(p) will display 'MyPlayer' here and Mypy
        # can properly type check its use.

        # ..and thus this code will properly be caught as an error.
        # "MyPlayer has no 'total_score' attribute"
        print(f'teammate {i} score is {p.total_score}')

# This function, however, uses unbound generics and is unsafe:
def print_teammate_scores_bad(player: ba.Player) -> None:
    for i, p in enumerate(player.team.players):

        # Here, 'player' has an unbound generic type: ba.Player[Any].
        # Therefore, the type checker doesn't know what type
        # 'player.team' is, much less 'player.team.players'.
        # Thus, reveal_type(p) will display 'Any' here.

        # ..and this broken code will not be caught until runtime
        # when it triggers an AttributeError.
        print(f'teammate {i} score is {p.total_score}')

So what is the lesson here?

It is not always obvious when code is unsafe due to unbound generics or types becoming Any. Ideally the user should look out for these situations, but they can be easy to miss.

To help in this process, code intended for reuse should be written defensively to avoid creating these scenarios as much as possible.

As a real-world example, let's create a class designed to be sent as a 'message' between Actors, Activities, etc. We will start by doing it the wrong way:

class PlayerIsAwesomeMessage:
    """Inform something that a specific player is, in fact, awesome."""
    def __init__(self, player: ba.Player) -> None:

        # We want these messages to be usable by any game type
        # no matter what player type they use, so we need
        # to accept/store ba.Player (ie: ba.Player[Any])
        self.player = player

# Code in an Activity, Actor, etc. might handle these messages in this way:
def handlemessage(msg: Any) -> None:
    if isinstance(msg, PlayerIsAwesomeMessage):
        
        # reveal_type(msg.player) will give 'ba.Player[Any]' here, and thus
        # reveal_type(msg.player.team) will give 'Any', which means the
        # following code is completely not type checked. If we later rename
        # or remove the 'awesomeness' attribute from the team, or simply
        # mistype it here, we will not know this code is broken until we
        # hit it at runtime.
        msg.player.team.awesomeness += 1

Technically we could improve type safety here be inserting an assert isinstance(msg.player, MyPlayer) before the last line, but most users would not think to do this and by default we would have silently un-typechecked code. So how do we improve this?

One might think we should make PlayerIsAwesomeMessage itself a generic class:

from typing import Generic, TypeVar

T = TypeVar('T', bound=ba.Player)

class PlayerIsAwesomeMessage(Generic[T]):
    """Inform something that this player is, in fact, awesome."""
    def __init__(self, player: T) -> None:
        self.player = player

# Ok, we've created a generic class; its 'T' type will be dependent
# on the type of player we pass it. 

msg = PlayerIsAwesomeMessage(myplayer)

# So if we do a reveal_type(msg) here, we will see
# 'PlayerIsAwesomeMessage[MyPlayer]' and reveal_type(msg.player)
# will give us 'MyPlayer'. So this 'msg' object is now type-safe.
# That's good.

# ...BUT...

# ...this doesn't actually help on the receiving end of our use case:
def handlemessage(msg: Any) -> None:
    if isinstance(msg, PlayerIsAwesomeMessage):
        # If you recall, extended type info such as generic
        # type-arguments is not available at runtime. In Python's eyes,
        # there is a single PlayerIsAwesomeMessage type, and we can't do
        # something like `isinstance(msg, PlayerIsAwesomeMessage[MyPlayer]).`
        # So at this point in the code, reveal_type(msg) will give us
        # 'PlayerIsAwesomeType[Any]', and reveal_type(msg.player) now gives
        # us 'Any', which is actually WORSE than our previous attempt which
        # at least gave us ba.Player[Any].
        msg.player.this.code.will.not.be.typechecked(':(')

        # We could, once again, do `assert isinstance(msg.player, MyPlayer)`
        # or use typing.cast() to get a strong type, but we want strong
        # types to happen by default, not through extra work.

So hmmm; what to do?

Thankfully there is a somewhat elegant solution for this scenario: require the user to pass a Type.

from typing import Type, TypeVar

T = TypeVar('T', bound=ba.Player)

class PlayerIsAwesomeMessage:
    """Inform something that this player is, in fact, awesome."""
    def __init__(self, player: ba.Player) -> None:

        # We still store a ba.Player[Any], but we make
        # it private so the user won't access it directly.
        self._player = player

    # Now, to access our player value, we simply
    # require the user to pass the player type they
    # are dealing with, and return our player as that
    # type. This will be a known constant for any given
    # game module so this isn't too ugly a thing to do.
    def getplayer(self, playertype: Type[T]) -> T:
        assert isinstance(self._player, playertype)
        return self._player

def handlemessage(msg: Any) -> None:
    if isinstance(msg, PlayerIsAwesomeMessage):

        # Now accessing the message's player is a bit
        # more verbose, but it 'forces' strong typing which
        # is a worthwhile tradeoff.
        msg.getplayer(MyPlayer).team.awesomeness += 1

        # So now reveal_type(msg.getplayer(MyPlayer)) gives
        # us 'MyPlayer' and all access is properly type
        # checked by default. Hooray!

The moral of this story is to try and encourage strong typing in a fashion that fits the use-case. Because messages are passed as opaque objects and player types are constant across a given game, explicitly asking for the player type is a good solution here. In other cases generic classes may be more elegant, or hard-coding a concrete player type might be. Just consider the situation and try to avoid propagating that dreaded Any.

C++ Guidelines

Base Guidelines

As much as possible, follow the advice in the Google C++ Style Guide except for the overrides noted below.

Exceptions, Clarifications, and Additions:

C++ Exceptions Allowed
Google's guidelines disallow the use of C++ exceptions largely in order to play nicely with existing non-exception-friendly code. Because we are a monolithic codebase and can ensure consistent exception handling throughout, we embrace their use.

<< Editor Setup Dependency System >>

Coding Style Guide - efroemling/ballistica GitHub Wiki

Motivation

Lint Checks

Python Guidelines

Base Guidelines

Exceptions, Clarifications, and Additions:

Auto-formatting is your friend.

Limit lines to 79 characters.

Avoid wildcard imports.

Importing classes/functions is ok.

Use descriptive public names.

It's ok to use staticmethod.

Don't explicitly inherit from object.

Use f-strings.

Avoid backslashes for line continuation.

Influence code layout with trailing commas.

Handle 'Existable' objects with care.

Expose complete, strong types by default.

C++ Guidelines

Base Guidelines

Exceptions, Clarifications, and Additions:

C++ Exceptions Allowed