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

Updating guide - parent/children communication and childrenspecs #474

Merged

Conversation

varsill
Copy link
Contributor

@varsill varsill commented Nov 15, 2022

Describe changes concerning:

  • Membrane.Time functions
  • parent/children communication
  • childrenspecs

@varsill varsill 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 Nov 15, 2022
@varsill varsill marked this pull request as ready for review November 15, 2022 09:38
@varsill varsill requested a review from mat-hek as a code owner November 15, 2022 09:39
Comment on lines 108 to 112
Change the signature of:
* `Membrane.Bin.handle_element_start_of_stream/3` into `Membrane.Bin.handle_element_start_of_stream/4`
* `Membrane.Bin.handle_element_end_of_stream/3` into `Membrane.Bin.handle_element_end_of_stream/4`
* `Membrane.Pipeline.handle_element_start_of_stream/3` into `Membrane.Pipeline.handle_element_start_of_stream/4`
* `Membrane.Pipeline.handle_element_end_of_stream/3` into `Membrane.Pipeline.handle_element_end_of_stream/4`
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
Change the signature of:
* `Membrane.Bin.handle_element_start_of_stream/3` into `Membrane.Bin.handle_element_start_of_stream/4`
* `Membrane.Bin.handle_element_end_of_stream/3` into `Membrane.Bin.handle_element_end_of_stream/4`
* `Membrane.Pipeline.handle_element_start_of_stream/3` into `Membrane.Pipeline.handle_element_start_of_stream/4`
* `Membrane.Pipeline.handle_element_end_of_stream/3` into `Membrane.Pipeline.handle_element_end_of_stream/4`

I'd remove that, I think that explanation below and the example is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, done

```


## Update the signatures of `handle_element_start_of_stream` and `handle_element_end_of_stream`
Copy link
Member

Choose a reason for hiding this comment

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

Since these are callbacks, I'd move them to the callbacks section, just mention that this applies to pipelines and bins only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

+ def handle_element_end_of_stream(child_name, pad_ref, context, state)
```

## Update functions used for children <-> parent communication
Copy link
Member

Choose a reason for hiding this comment

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

let's move that to callbacks and actions, respectively

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


## Update the children definitions
Children defintions syntax (previously known as `ParentSpec`, after the name of a structure used to define children), that was used in `Membrane.Pipeline.Action.spec_t` and `Membrane.Bin.Action.spec_t` actions, has changed.
Since there are quite a few changed concerning children definition, we have decided to present them in the subsections below.
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
Since there are quite a few changed concerning children definition, we have decided to present them in the subsections below.
Since there are quite a few changes concerning children definition, we have decided to present them in the subsections below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 180 to 181
+ children = [child(:source, SomeSource), child(:filter, SomeFilter)]
+ links = [get_child(:source) |> child(:another_filter, SomeFilter) |> get_child(:filter) |> child(:sink, SomeSink)]
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
+ children = [child(:source, SomeSource), child(:filter, SomeFilter)]
+ links = [get_child(:source) |> child(:another_filter, SomeFilter) |> get_child(:filter) |> child(:sink, SomeSink)]
+ structure = [
+ child(:source, SomeSource),
+ child(:filter, SomeFilter),
+ get_child(:source) |> child(:another_filter, SomeFilter) |> get_child(:filter) |> child(:sink, SomeSink)
+ ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 190 to 192
children = ...
links = ...
+ structure = children ++ links
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
children = ...
links = ...
+ structure = children ++ links

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 199 to 209
### Update the crash group definition
Crash groups has become inextricably connected with a new concept - `children groups`. The name of a crash group is now the name of a children group.
Update the following:
* remove the `crash_group: {crash_group_name, crash_group_mode}` option from the specification options
* add the `children_group_id` option to the specification options, with the value being the name of the former crash group: `children_group_id: crash_group_name`
* add the `crash_group_mode` option to the specification options, with the value being the mode of the former crash group: `crash_group_mode: crash_group_mode`

```diff
- options = [crash_group: {:first, :temporary}]
+ options = [children_group_id: :first, crash_group_mode: :temporary]
```
Copy link
Member

Choose a reason for hiding this comment

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

I think we decided to move that to 1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, I've removed that

Comment on lines 218 to 220
+ children = [child(:source, SomeSource), child(:filter, SomeFilter)]
+ links = [get_child(:source) |> child(:another_filter, SomeFilter) |> get_child(:filter) |> child(:sink, SomeSink)]
+ spec = {children ++ links, children_group_id: :first_group, crash_group_mode: :temporary, node: another_node}
Copy link
Member

Choose a reason for hiding this comment

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

As above, I would encourage creating both children and links in the same list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@varsill varsill requested a review from mat-hek November 16, 2022 14:00
@mat-hek mat-hek force-pushed the updating_guide/children_parent_communication_and_children_spec branch 2 times, most recently from e734266 to 6c49f78 Compare November 18, 2022 14:07
mat-hek and others added 2 commits November 21, 2022 10:41
* upgrade guide improvements

* upgrade guide: reorder callbacks

* upgrade guide: add bin_input and bin_output

* add makeup_diff to color diffs in docs

* update guide: put return values before callbacks

* updating guide: resolve conflicts

* Update guides/upgrading/v0.11.md

Co-authored-by: Łukasz Kita <lukasz.kita0@gmail.com>

Co-authored-by: Łukasz Kita <lukasz.kita0@gmail.com>
@mat-hek mat-hek merged commit 8995c4c into master Nov 21, 2022
@mat-hek mat-hek deleted the updating_guide/children_parent_communication_and_children_spec branch November 21, 2022 09:52
mat-hek added a commit that referenced this pull request Nov 21, 2022
* Describe changes concerning parent/children communication and childrenspecs

* Move the actions and callbacks changes description to the appropriate sections

* Update guides/upgrading/v0.11.md

Co-authored-by: Feliks Pobiedziński <38541925+FelonEkonom@users.noreply.github.com>

* Remove backtips from the function links which do not exists. Improve the n-ary function specification

* Add information about import Membrane.ChildrenSpec

* fix upgrade guide markdown syntax

* Add missing information about changes - handle_info, assert_pipeline_play and the changes in options of the testing pipeline

* Remove missusages of handle_other

* Update v0.11.md

* Updating guide: refactor (#480)

* upgrade guide improvements

* upgrade guide: reorder callbacks

* upgrade guide: add bin_input and bin_output

* add makeup_diff to color diffs in docs

* update guide: put return values before callbacks

* updating guide: resolve conflicts

* Update guides/upgrading/v0.11.md

Co-authored-by: Łukasz Kita <lukasz.kita0@gmail.com>

Co-authored-by: Łukasz Kita <lukasz.kita0@gmail.com>

Co-authored-by: Feliks Pobiedziński <38541925+FelonEkonom@users.noreply.github.com>
Co-authored-by: Mateusz Front <mateusz.front@swmansion.com>
Co-authored-by: Karol Konkol <56369082+Karolk99@users.noreply.github.com>
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.

4 participants