Skip to content
This repository has been archived by the owner on Jul 12, 2020. It is now read-only.

BaseInstance:Clone #180

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

BaseInstance:Clone #180

wants to merge 14 commits into from

Conversation

Kampfkarren
Copy link
Contributor

@Kampfkarren Kampfkarren commented Feb 13, 2019

Closes #143.

I like the idea behind how this is implemented, sort of an impl Clone feel. Looking for feedback on this design before moving forward.

Also thinking instead of a long if statement of every possible type, types implement their own :Clone functionality and if type(x) == "userdata" but typeof(x) ~= "userdata" then it attempts to call it.

Doesn't clone children yet, just want a comment about the design before going forward.

@coveralls
Copy link

coveralls commented Feb 13, 2019

Coverage Status

Coverage decreased (-0.002%) to 97.985% when pulling 557fbe7 on Kampfkarren:clone into 03b70f7 on LPGhatguy:master.

@LPGhatguy
Copy link
Owner

This design seems pretty nifty!

We should be careful not to clone instances unless they're children, I noticed that the default clone implementation clones Instances!

@Kampfkarren
Copy link
Contributor Author

I think I covered everything.

@Kampfkarren Kampfkarren changed the title [DNM] BaseInstance:Clone BaseInstance:Clone Feb 13, 2019
@LPGhatguy
Copy link
Owner

LPGhatguy commented Feb 13, 2019

Cloning objects like Vector3 is kind of strange because they're immutable since you can't rely on their hashing behavior and their equality is overridden!

@Kampfkarren
Copy link
Contributor Author

Kampfkarren commented Feb 13, 2019

Vector3s don't hash.

> v3v = Instance.new("Vector3Value")
> v3v.Value = Vector3.new()
> x = {[v3v.Value] = true}
> print(x[v3v.Value])
nil

And their equality isn't overriden. That was in the test. They have custom __eq behaviors.

function metatable:__eq(other)
return self.X == other.X and self.Y == other.Y and self.Z == other.Z
end

Enums do which is why they're just an identity function.

@Kampfkarren
Copy link
Contributor Author

luacov seems to think I didn't cover the if typeof(value) == "Instance" path even though I did?

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.

3 participants