Skip to content

Conversation

lnotspotl
Copy link
Member

This PR moves XLink nodes and XLink properties to a newly introduced dai::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.

@lnotspotl lnotspotl changed the title Lnotspotl/internal xlink Move XLink nodes nad XLink properties to a new dai::internal namespace May 18, 2025
@lnotspotl lnotspotl requested review from moratom and themarpe May 18, 2025 21:34
@lnotspotl lnotspotl self-assigned this May 18, 2025
@themarpe
Copy link
Collaborator

themarpe commented May 19, 2025

Great to see, having this internal scaffolding in place for non public/non stable API
LGTM from my side

@themarpe themarpe removed their request for review May 19, 2025 06:04
Copy link
Collaborator

@moratom moratom left a 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

Copy link
Collaborator

@moratom moratom left a 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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Member Author

@lnotspotl lnotspotl May 19, 2025

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

Copy link
Collaborator

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

Copy link
Collaborator

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

@lnotspotl
Copy link
Member Author

Will wait for the CI to finish and then I'll perform the merge

@lnotspotl lnotspotl merged commit c4a52f6 into v3_develop May 20, 2025
3 of 18 checks passed
@lnotspotl lnotspotl deleted the lnotspotl/internal_xlink branch May 20, 2025 13:35
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.

3 participants