-
Notifications
You must be signed in to change notification settings - Fork 154
Move XLink nodes nad XLink properties to a new dai::internal namespace #1323
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
Conversation
…-core into lnotspotl/internal_xlink
Great to see, having this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Two comments:
- We should remove the
v2_examples
(they are not in a working state anyways) - We don't need the bindings for XLink nodes anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add TODOs to tests that aren't ported rather than adding the internal namespace to them, but not critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we'd still want this? We can keep it in though, might come in handy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example doesn't compile/run anymore, so we can just add a TODO to port to v3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turns out the getPipeline
function wasn't used anywhere. I therefore removed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use some other node for that purpuse, no need to use XLinks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to add a TODO that it needs porting to v3
Will wait for the CI to finish and then I'll perform the merge |
This PR moves
XLink
nodes andXLink
properties to a newly introduceddai::internal
namespace. This shall be a namespace with necessary public implementation that is not meant to be used by the user due to its potentially unstable API. This PR should be reviewed alongside the other opened FW PRs on Gitlab. Changes necessary were minimal.