Recorder+Converter enhancements #197
Conversation
There was a problem hiding this comment.
Great job! 🎉
I didn't expect this code to expand so much 👀
One thing that comes to my mind is that we should move this to a separate package. Mostly because it brings a lot of optional dependencies and the logic (uploading, downloading to/from S3) is not strictly related to WebRTC. Initially, I thought this will be a small addition but it turned out to be pretty advanced.
Also, I believe this will help us make better API, a new package can have its own supervision tree where uploader tasks will be spawned, and will not affect casual webrtc development (tests duration, compilation time, etc.)
WDYT?
| report_path = | ||
| report_path | ||
| @spec convert_path!(Path.t(), options()) :: __MODULE__.Manifest.t() | no_return() | ||
| def convert_path!(recorder_manifest_path, options \\ []) do |
There was a problem hiding this comment.
Nitpick: I would call this convert and use pattern matching to see whether we got path or manifest as an argument
| end) | ||
|
|
||
| # FIXME: this links, ideally we should spawn a supervised task instead | ||
| task = Task.async(fn -> upload(manifest, bucket_name, s3_paths, s3_config_overrides) end) |
There was a problem hiding this comment.
You can use application supervision tree
|
@mickel8 I agree about the separation Closing this in favour of #199 and elixir-webrtc/ex_webrtc_recorder#1 |
2 participants
UPDATE
This PR has been closed in favour of #199 and elixir-webrtc/ex_webrtc_recorder#1