Refactor inventory data sending logic#524
Refactor inventory data sending logic#524Lucasgood5 wants to merge 1 commit intoNebulousCloud:masterfrom
Conversation
So Helix CI doesn't warn about this long line
There was a problem hiding this comment.
Pull request overview
Refactors ITEM:SetData’s inventory data replication logic to avoid a long conditional line (to satisfy Helix CI line-length warnings) in the item metadata layer.
Changes:
- Introduces a
targetlocal to hold the computed net receivers. - Replaces the inline
net.Send(...)receiver expression withnet.Send(target).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local inventory = ix.item.inventories[self.invID] | ||
|
|
||
| if (receivers != false and (receivers or inventory and inventory.GetReceivers and inventory:GetReceivers() or self:GetOwner())) then | ||
| local target = receivers or inventory and inventory.GetReceivers and inventory:GetReceivers() or self:GetOwner() |
There was a problem hiding this comment.
target is computed unconditionally. In the previous code, when receivers == false the and short-circuit prevented evaluating the receiver expression (including potentially expensive inventory:GetReceivers() / self:GetOwner() calls). With this refactor, those calls can still happen even though networking is disabled, which is a behavior/perf change. Consider moving the target computation inside the if (receivers != false ...) block (or guard it with if receivers != false then ... end) so the expression is only evaluated when it will be used.
| local target = receivers or inventory and inventory.GetReceivers and inventory:GetReceivers() or self:GetOwner() | |
| local target | |
| if (receivers != false) then | |
| target = receivers or inventory and inventory.GetReceivers and inventory:GetReceivers() or self:GetOwner() | |
| end |
| local target = receivers or inventory and inventory.GetReceivers and inventory:GetReceivers() or self:GetOwner() | ||
| if (receivers != false and target) then |
There was a problem hiding this comment.
PR description says there is no logic change, but computing target before the receivers != false check changes evaluation order and can trigger inventory:GetReceivers() / self:GetOwner() calls even when receivers == false. Either adjust the code to preserve the original short-circuit behavior, or update the PR description to reflect the behavioral change.
So Helix CI doesn't should no longer warn about this long line.
NB : i haven't changed the logic, i haven't even tried to understand the line. just make it so we go under the line lenght warning limit. Free to all to try to improve this.