Improving naming conventions

[Autsider666]

Context

I finally got the push to create this issue after reading this article by Samuel Asher Rivello, because I totally agree that a more consistent set of api/naming conventions.

Just looking at the Trigger shows so many examples where we've to spend some time to define what's "right" and what's "wrong":

• Constructor arguments being called options (Args, props, options and some more variants are used interchangeably all throughout the code)
• The type of the options using Partial instead of making properties nullable
• TriggerOptions being an interface instead of type
• 2-3 overlapping methods of setting "default" values for undefined arguments/options
• Interchangeable usage of Entity and Actor

Proposal

I'd suggest using this issue as an epic/hub for all the talks around naming conventions and create "sub" issues to tackle individual changes.

Last but not least: I suggest that these changes should be backwards compatible with 0.30, while deprecating everything for the version after it.

I quite enjoy diving into codebases to find/fix these kinds of inconsistencies, so I hope to start refactoring soon 😄

[mattjennings]

In my opinion the only way to 100% consistency is with automated tooling (linting). So if any of these are going to be addressed, they should come with an eslint rule. Otherwise we risk spending time cleaning things up only for them to likely slip by again because it was missed in a PR review.

With that being said:

Constructor arguments being called options (Args, props, options and some more variants are used interchangeably all throughout the code)

Agreed, but given its a param name it's not a huge issue IMO. not sure if there's a way to reasonably enforce this with eslint.

The type of the options using Partial instead of making properties nullable

at a glance this seems fine-ish, as all of those properties are required for a trigger whereas the user input (the partial) are optional. I suppose you could rename them to be more clear what is user input vs trigger properties.

TriggerOptions being an interface instead of type

there's going to be a lot of differing opinions on this (i personally prefer interface over type when possible because they have nicer ts error outputs). i don't think this is a big deal.

2-3 overlapping methods of setting "default" values for undefined arguments/options

agreed

Interchangeable usage of Entity and Actor

agreed

[Autsider666]

Quick addition to the list: Entity.get to get a specific component. I'd suggest changing it to getComponent

[Autsider666]

In my opinion the only way to 100% consistency is with automated tooling (linting). So if any of these are going to be addressed, they should come with an eslint rule. Otherwise we risk spending time cleaning things up only for them to likely slip by again because it was missed in a PR review.

You're 100% right, automated tooling would be required to enforce it in the long run. But should that stop us from improving the current codebase manually? Improving DX by applying consistent codestyle conventions to a 10+ year old codebase with the 1.0 release at the horizon.

P.s. I'm sorry for splitting it up in multiple smaller comments, but my memory is very limited, so this helps me focus on 1 thing at a time.

[Autsider666]

Constructor arguments being called options (Args, props, options and some more variants are used interchangeably all throughout the code)

Agreed, but given its a param name it's not a huge issue IMO. not sure if there's a way to reasonably enforce this with eslint.

The type of the options using Partial instead of making properties nullable

at a glance this seems fine-ish, as all of those properties are required for a trigger whereas the user input (the partial) are optional. I suppose you could rename them to be more clear what is user input vs trigger properties.

TriggerOptions being an interface instead of type

there's going to be a lot of differing opinions on this (i personally prefer interface over type when possible because they have nicer ts error outputs). i don't think this is a big deal.

The examples were meant to show the difference over multiple classes without calling any specific alternative "good" or "bad". The only "bad" thing (IMO) is that having this many viable options increases the cognitive load of interacting with the codebase, especially for new developers interested in contributing to EX or diving deeper into undocumented EX functionality.

A very personal example: I've git reset --harded over 10 attempts to help fix open issues, mostly because I just couldn't figure out how to implement/name stuff correctly. Partially because I'm a perfectionist with ADHD (like seemingly all of us 😁) and partially because of the vage and inconsistent Code StyleGuide in combination with all the different "conventions" throughout the codebase

[Autsider666]

Automatic using eslint would be a good start. Starting out with enabling eslint in the pipeline and getting the whole codebase up to the current eslint configuration wil probably already be a big undertaking, so I'd suggest creating a separate issue for it.

I'd suggest using something like eslint-bulk-suppressions in the meantime, to ignore the current eslint violations and focus on the new violation from new rules. (The automatic option in eslint to add eslint-ignore-line everwhere in the codebase is a far inferior alternative imo)

I've compiled a (non-exhaustive) list of eslint rules that could help automate some potential conventions:

• Prevent abreviations (eslint-plugin-unicorn)
• Max depth (eslint)
• Max lines per function (eslint)
• Potentially more "max-*" rules. Won't list them all here.
• Naming convention (typescript-eslint) (Already in .eslintrc.json)
• No Shadow (typescript-eslint)
• Start with a verb (eslint-plugin-function-name)
• (Cyclomatic) Complexity (eslint)
• No nested ternary (eslint) (Haven't encountered any in EX, so let's enforce it)

I'd suggest adding one rule at a time to keep PR's small and easier to review.

For in the future: Extending recommended-type-checked or maybe even strict-type-checked from typescript-eslint.

[github-actions[bot]]

This issue hasn't had any recent activity lately and is being marked as stale automatically.