Skip to content

[RTC-192] RTSP component + small refactor#9

Merged
sgfn merged 3 commits intomasterfrom
sgfn/RTC-192-rtsp-component
May 9, 2023
Merged

[RTC-192] RTSP component + small refactor#9
sgfn merged 3 commits intomasterfrom
sgfn/RTC-192-rtsp-component

Conversation

@sgfn
Copy link
Copy Markdown
Member

@sgfn sgfn commented Apr 24, 2023

This PR introduces breaking changes to the creation of components

@sgfn sgfn requested review from LVala and mickel8 April 24, 2023 10:59
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 24, 2023

Codecov Report

Merging #9 (eac3832) into master (822c8a9) will decrease coverage by 0.84%.
The diff coverage is 63.15%.

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage   58.40%   57.57%   -0.84%     
==========================================
  Files           7        7              
  Lines         113      132      +19     
==========================================
+ Hits           66       76      +10     
- Misses         47       56       +9     
Impacted Files Coverage Δ
lib/jellyfish/room.ex 77.58% <33.33%> (-8.96%) ⬇️
lib/jellyfish/component.ex 70.00% <71.42%> (+3.33%) ⬆️
lib/jellyfish/peer.ex 77.77% <83.33%> (+11.11%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 822c8a9...eac3832. Read the comment docs.

Comment thread lib/jellyfish/component.ex Outdated
@sgfn sgfn requested review from LVala and mickel8 May 5, 2023 10:36
Comment thread lib/jellyfish/component.ex Outdated
end

@doc false
@spec type_from_options(struct()) :: atom()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@spec type_from_options(struct()) :: atom()
@spec type_from_options(struct()) :: type()

Comment thread lib/jellyfish/component.ex Outdated
end

@doc false
@spec type_from_string(String.t()) :: atom()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@spec type_from_string(String.t()) :: atom()
@spec type_from_string(String.t()) :: type()

Comment thread lib/jellyfish/peer.ex Outdated
end

@doc false
@spec type_from_string(String.t()) :: atom()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@spec type_from_string(String.t()) :: atom()
@spec type_from_string(String.t()) :: type()

def type_from_string(string) do
if string in @valid_type_strings,
do: String.to_atom(string),
else: raise("Invalid component type string")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

By convention, we call raise without parentheses (as you did in some other places).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm blaming mix format for this :PP

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, it keeps adding the parentheses -- I'm afraid the mighty linter has spoken...

I guess the convention doesn't apply when using keyword list if syntax ¯_(ツ)_/¯

Comment on lines +9 to +23
defstruct @enforce_keys ++
[
rtpPort: 20_000,
reconnectDelay: 15_000,
keepAliveInterval: 15_000,
pierceNat: true
]

@type t :: %__MODULE__{
sourceUri: URI.t(),
rtpPort: 1..65_535,
reconnectDelay: non_neg_integer(),
keepAliveInterval: non_neg_integer(),
pierceNat: boolean()
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Noticed it only now, you should use snake_case here and create a function that converts it to camelCase like we do e.g. here. Hope that won't be an issue anymore when we move to protobufs.

@mickel8 mickel8 self-requested a review May 8, 2023 08:38
Comment thread test/jellyfish/room_test.exs Outdated
@sgfn sgfn requested review from LVala and mickel8 May 8, 2023 11:16
Copy link
Copy Markdown
Collaborator

@LVala LVala left a comment

Choose a reason for hiding this comment

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

👍

@sgfn sgfn merged commit 96580aa into master May 9, 2023
@sgfn sgfn deleted the sgfn/RTC-192-rtsp-component branch May 9, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants