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

Generate warnings related to pads with :auto flow control #900

Merged
merged 13 commits into from
Nov 20, 2024

Conversation

FelonEkonom
Copy link
Member

@FelonEkonom FelonEkonom commented Oct 29, 2024

Closes #868
Closes #869

@FelonEkonom FelonEkonom self-assigned this Oct 29, 2024
@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 Oct 29, 2024
@FelonEkonom FelonEkonom changed the title Warn on multimple auto input and auto output pads at the same time Generate warnings related to pads with :auto flow control Oct 29, 2024
Copy link
Member

@mat-hek mat-hek left a comment

Choose a reason for hiding this comment

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

I'm not sure that warnings are the best way to inform a user about behaviour that may be completely desired. IMO warnings should indicate that even though something won't necessarily break, it's definitely wrong, which is not the case here. That said, I have no idea for a better way to solve this :P

env.module
|> Module.get_attribute(:__membrane_element_type__, :bin)
|> case do
:bin -> Membrane.Bin
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this apply to elements only?

@@ -25,6 +25,9 @@ defmodule Membrane.Endpoint do

Options:
- `:bring_pad?` - if true (default) requires and aliases `Membrane.Pad`
- `:flow_control_related_warnings?` - if true (default) generates compile-time warnings \
Copy link
Member

Choose a reason for hiding this comment

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

What about

Suggested change
- `:flow_control_related_warnings?` - if true (default) generates compile-time warnings \
- `:flow_control_hints?` - if true (default) generates compile-time warnings \

@FelonEkonom FelonEkonom merged commit ca055af into master Nov 20, 2024
5 of 6 checks passed
@FelonEkonom FelonEkonom deleted the warn-on-many-input-and-output-auto-pads branch November 20, 2024 18:14
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
2 participants