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.

Sign up for GitHub

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

Jump to bottom

Implement :remove_link action #487

Merged
merged 14 commits into from
Dec 15, 2022
Merged

Implement :remove_link action #487

merged 14 commits into from
Dec 15, 2022

Conversation

FelonEkonom
Copy link
Member

@FelonEkonom FelonEkonom commented Dec 5, 2022 •
edited
Loading

@FelonEkonom FelonEkonom self-assigned this Dec 5, 2022
@FelonEkonom FelonEkonom force-pushed the remove-link branch 2 times, most recently from c8d180d to 5935e9e Compare December 5, 2022 17:12
@FelonEkonom FelonEkonom requested a review from varsill December 6, 2022 13:09
@FelonEkonom FelonEkonom added the no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md label Dec 6, 2022
@FelonEkonom FelonEkonom marked this pull request as ready for review December 6, 2022 13:25
@FelonEkonom FelonEkonom requested a review from mat-hek as a code owner December 6, 2022 13:25
varsill
varsill requested changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question - what is the expected behaviour when removing a link, with at least one endpoint being a static pad? Shouldn't we disallow such unlinking and raise some meaningful exception?
Nevertheless, I believe we should check that case in tests.

lib/membrane/bin/action.ex Outdated
@@ -37,6 +37,11 @@ defmodule Membrane.Bin.Action do
Child.name_t()
| [Child.name_t()]}

@typedoc """
Actions that removes link, that realates to specified child and pad.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Actions that removes link, that realates to specified child and pad.
Action that removes link, which relates to specified child and pad.

lib/membrane/pipeline/action.ex Outdated
@@ -29,6 +29,11 @@ defmodule Membrane.Pipeline.Action do
@type remove_child_t ::
{:remove_child, Child.name_t() | [Child.name_t()]}

@typedoc """
Actions that removes link, that relates to specified child and pad.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Actions that removes link, that relates to specified child and pad.
Action that removes link, which relates to specified child and pad.

test/membrane/integration/linking_test.exs Outdated
end
end

defp assert_pad_removed(pipeline, id) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this function should be called assert_link_removed (and the function below should be refute_link_removed)

@FelonEkonom
Copy link
Member Author

General question - what is the expected behaviour when removing a link, with at least one endpoint being a static pad? Shouldn't we disallow such unlinking and raise some meaningful exception? Nevertheless, I believe we should check that case in tests.

@varsill if you will try to unlink static pad using :remove_link action, element/bin with static pad will raise Membrane.PadError with message "Tried to unlink a static pad #{inspect(pad_ref)}. Static pads cannot be unlinked unless element is terminating"

@FelonEkonom FelonEkonom added enhancement and removed no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md labels Dec 7, 2022
@FelonEkonom FelonEkonom requested a review from varsill December 7, 2022 12:55
@varsill
Copy link
Contributor

varsill commented Dec 7, 2022 •
edited
Loading

General question - what is the expected behaviour when removing a link, with at least one endpoint being a static pad? Shouldn't we disallow such unlinking and raise some meaningful exception? Nevertheless, I believe we should check that case in tests.

@varsill if you will try to unlink static pad using :remove_link action, element/bin with static pad will raise Membrane.PadError with message "Tried to unlink a static pad #{inspect(pad_ref)}. Static pads cannot be unlinked unless element is terminating"

Ok, that seems fair enough ;) Probably we don't need to test that though

mat-hek
mat-hek requested changes Dec 14, 2022
View reviewed changes
test/membrane/integration/linking_test.exs Outdated
Comment on lines 499 to 503
if pad_id < 10 do
for i <- (pad_id + 1)..10 do
refute_link_removed(pipeline, i)
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if pad_id < 10 do
for i <- (pad_id + 1)..10 do
refute_link_removed(pipeline, i)
end
end
for i <- (pad_id + 1)..10//1 do
refute_link_removed(pipeline, i)
end

lib/membrane/core/parent/child_life_controller/link_utils.ex Outdated

nil ->
raise LinkError, """
Attempted to unlink pad #{inspect(pad_ref)} of child #{inspect(child_name)}, but this child does not have this pad
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Attempted to unlink pad #{inspect(pad_ref)} of child #{inspect(child_name)}, but this child does not have this pad
Attempted to unlink pad #{inspect(pad_ref)} of child #{inspect(child_name)}, but no such link exists

We should probably first check if such a child exists to give a better error when it doesn't

mat-hek
mat-hek requested changes Dec 14, 2022
View reviewed changes
lib/membrane/bin/action.ex
@@ -42,6 +42,11 @@ defmodule Membrane.Bin.Action do
| Membrane.Child.group_t()
| [Membrane.Child.group_t()]}

@typedoc """
Action that removes link, which relates to specified child and pad.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's note that only dynamic pads can be unlinked

mat-hek
mat-hek requested changes Dec 15, 2022
View reviewed changes
lib/membrane/core/parent/child_life_controller/link_utils.ex Outdated
Comment on lines 52 to 56
with %{^child_name => _child_entry} <- state.children do
raise ParentError, """
Attempted to unlink pad #{inspect(pad_ref)} of child #{inspect(child_name)}, but such a child does not exist
"""
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this check the opposite?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes ;P

@FelonEkonom FelonEkonom requested a review from mat-hek December 15, 2022 13:01
@FelonEkonom FelonEkonom merged commit 7aebd1d into master Dec 15, 2022
@FelonEkonom FelonEkonom deleted the remove-link branch December 15, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mat-hek mat-hek mat-hek approved these changes

@varsill varsill varsill approved these changes

Assignees

@FelonEkonom FelonEkonom

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FelonEkonom @varsill @mat-hek