-
Notifications
You must be signed in to change notification settings - Fork 35
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
Raise on attempting to remove a not existing child #553
Conversation
cb75224
to
39281a4
Compare
|> List.delete(nil) | ||
|> Enum.uniq() |
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.
|> List.delete(nil) | |
|> Enum.uniq() | |
|> Enum.uniq() | |
|> List.delete(nil) |
|> Enum.uniq() | ||
|
||
children_or_children_groups | ||
|> Enum.find(&(&1 not in all_children_and_children_groups)) |
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.
instead of searching through all children, we could just search in the ones that are to be removed
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.
value refs
in line 522 contains only references to the children that we have to remove, while variable children_or_children_groups
might contain not only children refs, but child groups names as well, so I am not convinced if this change will make the code simpler
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.
but we can easily get the groups when retrieving refs
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 have moved part of code checking whether removed child exist to the another function, IMO this is cleaner then previous version
8f566f3
to
2137940
Compare
Enum.find(removed_children_or_groups, fn name -> | ||
not Map.has_key?(state.children, name) and not MapSet.member?(children_groups, name) | ||
end) | ||
|> case do | ||
nil -> | ||
:ok | ||
|
||
child_ref -> | ||
raise Membrane.ParentError, """ | ||
Trying to remove child #{inspect(child_ref)}, while such a child or children group does not exist. |
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.
Enum.find(removed_children_or_groups, fn name -> | |
not Map.has_key?(state.children, name) and not MapSet.member?(children_groups, name) | |
end) | |
|> case do | |
nil -> | |
:ok | |
child_ref -> | |
raise Membrane.ParentError, """ | |
Trying to remove child #{inspect(child_ref)}, while such a child or children group does not exist. | |
removed_children_or_groups | |
|> Enum.reject(fn name -> | |
Map.has_key?(state.children, name) or MapSet.member?(children_groups, name) | |
end) | |
|> case do | |
[] -> | |
:ok | |
children_refs -> | |
raise Membrane.ParentError, """ | |
Trying to remove children #{Enum.map_join(", ", &inspect/1)}, while such children or children groups do not exist. |
Related Jira ticket: https://membraneframework.atlassian.net/browse/MC-176