Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Follow style guide + Luau performance guide #294

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

howmanysmall
Copy link
Contributor

@howmanysmall howmanysmall commented Jan 14, 2021

I've made the following changes to the codebase, as seen in the changelog.

  • Fixed several linting problems.
  • Made a few small performance changes (reducing indexing, checking optional values being nil before type checking them, using string.x instead of :x).
  • Made the code better follow the Roblox Lua Style Guide.
  • Changed some of the examples to also follow the Lua style guide better.

Checklist before submitting:

  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

Copy link
Contributor

@matthargett matthargett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! A few minor requests, and then we can review again.

@@ -379,7 +372,7 @@ local function createReconciler(renderer)
local function mountVirtualTree(element, hostParent, hostKey)
if config.typeChecks then
assert(Type.of(element) == Type.Element, "Expected arg #1 to be of type Element")
assert(renderer.isHostObject(hostParent) or hostParent == nil, "Expected arg #2 to be a host object")
assert(hostParent == nil or renderer.isHostObject(hostParent), "Expected arg #2 to be a host object")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you're changing these a bit, can you also make them say something more specific like:
"Expected the hostParent argument to mountVirtualTree to be a Host Object."?

@@ -40,9 +42,9 @@ return function()
local TextReverser = Roact.Component:extend("TextReverser")

function TextReverser:init()
self.state = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may be changing our guidance on this in the future, so leave this one for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants