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

Refactor child pad removed default error message #581

Merged
merged 6 commits into from
Jul 21, 2023

Conversation

FelonEkonom
Copy link
Member

@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 Jul 19, 2023
callback_ref = "`c:Membrane.#{component_type_string}.handle_child_pad_removed/4`"

"""
Child #{inspect(child)} removed its pad #{inspect(pad)}, but callback #{callback_ref} is not implemented in #{inspect(component_module)}.
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
Child #{inspect(child)} removed its pad #{inspect(pad)}, but callback #{callback_ref} is not implemented in #{inspect(component_module)}.
Bin #{inspect(child)} removed its pad #{inspect(pad)}, but callback #{callback_ref} is not implemented in #{inspect(component_module)}.

"""
Child #{inspect(child)} removed its pad #{inspect(pad)}, but callback #{callback_ref} is not implemented in #{inspect(component_module)}.

This means, that `#{inspect(child)} is a bin, that removed its pad #{inspect(pad)} on its own, without knowledge of its parent. It
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
This means, that `#{inspect(child)} is a bin, that removed its pad #{inspect(pad)} on its own, without knowledge of its parent. It
This means, that `#{inspect(child)} removed the pad on its own, without knowledge of its parent. It

could be done, by, for example, removing #{inspect(child)}'s child linked to the #{inspect(child)}'s inner pad or by removing link
between #{inspect(child)} and its child.

If you want to handle e scenario when a child removes its pad in #{inspect(component_module)}, implement #{callback_ref} callback.
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 you want to handle e scenario when a child removes its pad in #{inspect(component_module)}, implement #{callback_ref} callback.
If you want to handle this scenario, implement #{callback_ref} callback in #{inspect(component_module)}.

Comment on lines 20 to 21
could be done, by, for example, removing #{inspect(child)}'s child linked to the #{inspect(child)}'s inner pad or by removing link
between #{inspect(child)} and its child.
Copy link
Member

@mat-hek mat-hek Jul 19, 2023

Choose a reason for hiding this comment

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

Suggested change
could be done, by, for example, removing #{inspect(child)}'s child linked to the #{inspect(child)}'s inner pad or by removing link
between #{inspect(child)} and its child.
may have happened because, for example, #{inspect(child)} removed its child linked to its inner pad or unlinked its child from its inner pad.

@FelonEkonom FelonEkonom force-pushed the refactor-child-pad-removed-error-message branch from 9cd94d2 to 7f6ac6d Compare July 20, 2023 12:41
lib/membrane/exceptions.ex Outdated Show resolved Hide resolved
Co-authored-by: Łukasz Kita <lukasz.kita0@gmail.com>
@FelonEkonom FelonEkonom requested a review from varsill July 20, 2023 13:34
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.

Just consider my last suggestion ;)

msg = """
Bin #{inspect(child)} removed its pad #{inspect(pad)}, but callback #{callback_ref} is not implemented in #{inspect(module)}.

This means, that `#{inspect(child)} removed the pad on its own, without knowledge of its parent. For example it could happen due to removing #{inspect(child)}'s child linked to the #{inspect(child)}'s inner pad or by removing link between #{inspect(child)} and its child.
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we split it into two lines?

@FelonEkonom FelonEkonom merged commit 28622f9 into master Jul 21, 2023
@FelonEkonom FelonEkonom deleted the refactor-child-pad-removed-error-message branch July 21, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants