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

Bump Ratio to 3.0 #438

Merged
merged 7 commits into from
Dec 1, 2022
Merged

Bump Ratio to 3.0 #438

merged 7 commits into from
Dec 1, 2022

Conversation

varsill
Copy link
Contributor

@varsill varsill commented Jul 12, 2022

This PR updates the Ratio dependency do 3.0 and updates the code accordingly to the changes in API. This PR should be merged once we decide to update Ratio dependency in all the plugins in organization.

@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 Jul 12, 2022
@varsill
Copy link
Contributor Author

varsill commented Nov 15, 2022

Closing since Ratio 4.0 has been released ;)

@varsill varsill closed this Nov 15, 2022
@varsill varsill reopened this Nov 24, 2022
@varsill
Copy link
Contributor Author

varsill commented Nov 24, 2022

Reopening since Ratio v4.0.0 is only a release candidate ;)

@varsill varsill marked this pull request as ready for review November 24, 2022 10:28
Comment on lines 215 to 217
use Numbers, overload_operators: true

if till_next < 0 do
if Ratio.lt?(till_next, 0) do
Copy link
Member

Choose a reason for hiding this comment

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

why do we overload operators if we don't use them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we are using them - the overloaded operators are not only comparison operators, but also arithmetic operators

Comment on lines 230 to 233
use Numbers, overload_operators: true
%{till_next: from_previous, clock_time: clock_time} = state
clock_time = clock_time + from_previous
ratio = clock_time / (state.time_provider.() - state.init_time)
ratio = Ratio.div(Ratio.new(clock_time), Ratio.new(state.time_provider.() - state.init_time))
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

@varsill varsill requested a review from mat-hek November 25, 2022 16:26
Comment on lines 37 to 41
@typedoc """
Ratio of the Clock time to the reference time.
"""
@type ratio_t :: Ratio.t() | non_neg_integer

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave this type

Suggested change
@typedoc """
Ratio of the Clock time to the reference time.
"""
@type ratio_t :: Ratio.t() | non_neg_integer
@typedoc """
Ratio of the Clock time to the reference time.
"""
@type ratio_t :: Ratio.t()

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

clock_time = clock_time + from_previous
ratio = clock_time / (state.time_provider.() - state.init_time)
clock_time = Ratio.add(clock_time, from_previous)
ratio = Ratio.new(clock_time, state.time_provider.() - state.init_time)
Copy link
Member

Choose a reason for hiding this comment

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

div is less confusing in this case IMO

Suggested change
ratio = Ratio.new(clock_time, state.time_provider.() - state.init_time)
ratio = Ratio.div(clock_time, state.time_provider.() - state.init_time)

Copy link
Contributor Author

@varsill varsill Nov 29, 2022

Choose a reason for hiding this comment

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

done
The reason why Ratio.new was used there previously is because Ratio.new accepts both integers and Ratios as arguments, while Ratio.div enforces usage of Ratio - I need to convert state.time_provider.() - state.init_time into a Ratio by using Ratio.new anyway :D

@@ -48,7 +48,7 @@ defmodule Membrane.Core.Timer do
end

def tick(timer) do
use Ratio
use Numbers, overload_operators: true
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 be consistent and not use that

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

raise "Only integers and rationals can be converted with Membrane.Time.#{unquote(unit.plural)}"
end
def unquote(unit.plural)(number) when Ratio.is_rational(number) do
use Numbers, overload_operators: true
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

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 here and also in clock_test.exs

@varsill varsill requested a review from mat-hek November 29, 2022 08:57
@varsill varsill merged commit d05b7fd into master Dec 1, 2022
@varsill varsill deleted the bump_ratio_to_v3.0 branch December 1, 2022 10:58
FelonEkonom added a commit that referenced this pull request Dec 7, 2022
FelonEkonom added a commit that referenced this pull request Apr 12, 2023
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.

2 participants