Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add rule for creating node child within a loop #123

Open
wants to merge 1 commit into
base: v1
Choose a base branch
from

Conversation

RokuAndrii
Copy link
Contributor

@RokuAndrii RokuAndrii commented Jul 29, 2024

image

Copy link
Member

@TwitchBronBron TwitchBronBron left a comment

Choose a reason for hiding this comment

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

Sorry for the delay reviewing this, but I have been thinking about it for a while, I finally had a chance to assemble my thoughts.

First of all, this rule as it is right now now is a little confusing. The explanation Do not use in a loop to append multiple children. Use CreateChildren() instead makes this rule seem like you're not supposed to call CreateChild in a loop at all, but then the actual diagnostics are only warning about setting properties on a node created in a loop.

Here's how I think we should move forward here:
Let's split this into 2 PRs.

PR 1 - no CreateChild() in loop

This current PR would implement the following logic, which I believe was your original intent:
Flag any usage of CreateChild() in a loop. The rule description should explain that you should instead prefer calling CreateChildren() outside the loop.

PR 2 - no multi-assignment to a component, prefer using update() instead

This is what your PR currently seems to enforce. It's going to be tricky to properly enforce this, as there are significantly more "valid" use cases for setting component props than bad cases. You could set properties in if statements, nested loops, etc. And the order of property setting sometimes matters too.

If you're still interested in implementing this rule, I think it should be restricted as follows:

  • when setting consecutive properties on a node, flag the issue. Here's an example:
someNode = m.top
someNode.prop1 = true
someNode.prop2 = true
       ~~~~~ avoid setting multiple props on a node, use `.update()` instead.
someNode = getOtherNode()
someNode.prop1 = true
someNode = getOtherNode2()
someNode.prop2 = true
someNode.prop3 = true
        ~~~~~ avoid setting multiple props on a node, use `.update()` instead.
if true
    someNode.prop1 = true
end if
someNode.prop1 = true

I'm open to discussing all of this, but these are my initial thoughts.

diagnostics.push(messages.NoCreateChildInLoop(s.tokens.name.location.range, noCreateObjectInLoop));
}
}
}), { walkMode: WalkMode.visitAllRecursive });
Copy link
Member

Choose a reason for hiding this comment

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

Don't use visitAllRecursive here. That would step into nested function bodies, which we don't want. I think you can use visitStatements since you're only looking at DottedSetStatements

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

Successfully merging this pull request may close these issues.

2 participants