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

Catching double child assignment #108

Open
dphfox opened this issue Dec 24, 2021 · 3 comments
Open

Catching double child assignment #108

dphfox opened this issue Dec 24, 2021 · 3 comments
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made

Comments

@dphfox
Copy link
Owner

dphfox commented Dec 24, 2021

Currently in Fusion, the following code will not error, and place both instances in folder1. This is because [Children] doesn't override a manual Parent definition:

local folder1 = New "Folder" {}

local instance1 = New "Part" {
    Parent = folder1
}

local instance2 = New "Part" {
    Parent = Value(folder1)
}

local folder2 = New "Folder" {
    [Children] = {instance1, instance2}
}

The following code also doesn't throw an error, but the parent of instance1 is undefined and prone to switching dependent on the internal implementation of Fusion:

local instance1 = New "Part" {}

local folder1 = New "Folder" {
    [Children] = {instance1}
}

local folder2 = New "Folder" {
    [Children] = {instance1}
}

I think that in any case where an instance's ancestry is defined twice, we should throw an error, including cases where we currently allow it by giving Parent priority. This is a breaking change but it's likely a really bad idea to introduce this kind of ambiguity into a codebase anyway when everything is meant to be declarative, final and guaranteed to hold at all times.

@dphfox dphfox added broken Something isn't right not ready - evaluating Currently gauging feedback labels Dec 24, 2021
@dphfox dphfox added enhancement New feature or request and removed broken Something isn't right labels Dec 24, 2021
@nottirb
Copy link
Contributor

nottirb commented Dec 24, 2021

it's likely a really bad idea to introduce this kind of ambiguity into a codebase anyway when everything is meant to be declarative, final and guaranteed to hold at all times

If you're defining an instance's ancestry twice, it can lead to some interesting issues, and I can't think of any good use cases for doing that anyways.

Additionally, if you're re-using some part of your UI you typically should be using components. If anything, erroring on the second example forces people to think about how they're setting up their UI.

For that reason I think this would be a good addition.

@dphfox dphfox added ready to work on Enhancements/changes ready to be made and removed not ready - evaluating Currently gauging feedback labels Jan 15, 2022
@dphfox dphfox added this to Fusion 0.3 Feb 1, 2023
@dphfox dphfox moved this to To Do in Fusion 0.3 Feb 1, 2023
@dphfox
Copy link
Owner Author

dphfox commented Feb 1, 2023

Waiting for #206 to see how we should implement this.

@dphfox dphfox added not ready - blocked Waiting on other work to be done and removed ready to work on Enhancements/changes ready to be made labels Feb 1, 2023
@dphfox dphfox changed the title Catching double parent/child assignment Catching double child assignment Apr 15, 2024
@dphfox
Copy link
Owner Author

dphfox commented Apr 15, 2024

Unblocking this. We won't need double parent assignment - that should be impossible now - but it's still possible to attempt to parent one child into two places. I'm not sure how urgent this is though, so I'm probably not going to tackle it until down the road.

@dphfox dphfox added ready to work on Enhancements/changes ready to be made and removed not ready - blocked Waiting on other work to be done labels Apr 15, 2024
@dphfox dphfox removed this from Fusion 0.3 Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants