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

Use Sequence lengths when determining validity for connections #2976

Closed
wants to merge 1 commit into from

Conversation

joeyballentine
Copy link
Member

image

@RunDevelopment
Copy link
Member

This check is trivial to defeat in 2 ways:

  1. By having a regular node in between
    image
  2. By having the connection be valid at first, and then changing the sequence length of either node to make it invalid.
    image

@joeyballentine
Copy link
Member Author

making sequence length types get passed down to things you have connected sounds like a separate effort / separate PR to me

The other part sounds like I just have to do this same kind of check in the other place we're checking validity? But I thought it used the same check for both? I'll have to do some digging

@joeyballentine
Copy link
Member Author

Yeah I'll be honest here -- I just tried to do the second suggestion and I cannot figure it out. I'm confused on how the current input errors even work, and how i could do something similar for sequence mismatches. There doesn't seem to be any input-to-output matching going on and yet somehow it knows the types are incompatible. I don't get it

@joeyballentine
Copy link
Member Author

TBH, both of these things feel like they should be separate efforts. This just adds the validation for connectivity (since there's 0 validation right now), and in separate PRs we can do what's necessary for getting these other things working

@joeyballentine
Copy link
Member Author

Those two concerns are now addressed in #2996 and #2997

@RunDevelopment
Copy link
Member

Blocked by #2996.

This PR doesn't work correctly until #2996, so I won't merge this PR until I can test it.

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