added :default_formatter option parameter and Membrane.Time.pretty_du… #134

mat-hek · 2018-12-14T19:02:36Z

…ration/1

bblaszkow06 · 2018-12-15T19:17:22Z

lib/membrane/element/base/mixin/common_behaviour.ex

@@ -189,6 +189,8 @@ defmodule Membrane.Element.Base.Mixin.CommonBehaviour do
      For others typespec is set to `t:any/0`
    * `default:` default value for option. If not present, value for this option
      will have to be provided each time options struct is created
+    * `default_formatter:` function converting value provided as `default` to string.


to a string

bblaszkow06 · 2018-12-15T19:21:13Z

lib/membrane/element/base/mixin/common_behaviour.ex

@@ -189,6 +189,8 @@ defmodule Membrane.Element.Base.Mixin.CommonBehaviour do
      For others typespec is set to `t:any/0`
    * `default:` default value for option. If not present, value for this option
      will have to be provided each time options struct is created
+    * `default_formatter:` function converting value provided as `default` to string.
+      Used upon creating documentation instead of `inspect/1`


'upon' sounds weird to me, I don't think it's the correct word. How about when?

bblaszkow06 · 2018-12-15T19:27:28Z

lib/membrane/element/base/mixin/common_behaviour.ex

@@ -203,8 +205,11 @@ defmodule Membrane.Element.Base.Mixin.CommonBehaviour do

        default_val_desc =
          if Keyword.has_key?(v, :default) do
+            default_formatter =
+              v |> Keyword.get(:default_formatter, quote(do: &"`#{inspect(&1)}`"))


Do you think it should be allowed to use formatter producing string without backticks? I think they should be enforced and therefore placed inside a string on line 212. This default value would be simplified to &inspect/1

Backticks suggest that there's a term between them, while formatter may return kind of description of what the default value is, which may not be a term. For example '2 ms' returned by Membrane.Time.pretty_duration/1 shouldn't be considered a term to me.

bblaszkow06 · 2018-12-15T19:29:13Z

lib/membrane/time.ex

@@ -60,6 +60,34 @@ defmodule Membrane.Time do
  """
  defguard is_native_t(value) when is_integer(value)

+  @doc """
+  Returns duration as a string with unit. Chosen unit is the biggest possible one


one at the end seems redundant

bblaszkow06 · 2018-12-15T20:29:21Z

lib/membrane/time.ex

+
+  ## Examples
+
+      iex> 10 |> #{inspect(__MODULE__)}.milliseconds() |> #{inspect(__MODULE__)}.pretty_duration()


You can format the tests a bit nicer:

iex> 10 ...> |> #{inspect(__MODULE__)}.milliseconds() ...> |> #{inspect(__MODULE__)}.pretty_duration() "10 ms" iex> 60_000_000 ...> |> #{inspect(__MODULE__)}.microseconds() ...> |> #{inspect(__MODULE__)}.pretty_duration() "1 min"

bblaszkow06 · 2018-12-15T20:30:50Z

lib/membrane/time.ex

+      iex> 10 |> #{inspect(__MODULE__)}.milliseconds() |> #{inspect(__MODULE__)}.pretty_duration()
+      "10 ms"
+      iex> 60_000_000 |> #{inspect(__MODULE__)}.microseconds() |> #{inspect(__MODULE__)}.pretty_duration()
+      "1 min"


Add a test for days like this to at least ensure the whole array is traversed:

iex> 6 ...> |> #{inspect(__MODULE__)}.days() ...> |> #{inspect(__MODULE__)}.pretty_duration() "6 d"

bblaszkow06 · 2018-12-15T20:41:18Z

lib/membrane/time.ex

+  """
+  @spec pretty_duration(t) :: String.t()
+  def pretty_duration(time) when is_t(time) do
+    [ns: 1000, us: 1000, ms: 1000, s: 60, min: 60, h: 24]


As we've discussed, I don't like this function very much, but it's OK. Anyway, here's my solution:

def pretty_duration(time) when is_t(time) do [ns: 1, us: 1000, ms: 1000, s: 1000, min: 60, h: 60, d: 24] |> Enum.scan(fn {unit, mult}, {_, acc} -> {unit, acc * mult} end) |> Enum.reverse() |> Enum.find_value(fn {unit, divisor} -> rem(time, divisor) == 0 && "#{time |> div(divisor)} #{unit}" end) end

I know it's a bit less efficient, but takes half of the lines compared to yours 😛
Do whatever you want with it 😄

bblaszkow06 · 2018-12-17T10:03:17Z

Backticks are not just for terms but for anything you want to present using monospace font. In docs I usually put values like this in backticks. In this case I would also use them to keep the style for different options cosistent. Anyway, It's a matter of opinion so we probably need a 3rd vote 😉 pon., 17 gru 2018, 10:55: Mateusz Front <notifications@github.com> napisał(a):

…

***@***.**** commented on this pull request. ------------------------------ In lib/membrane/element/base/mixin/common_behaviour.ex <#134 (comment)> : > @@ -203,8 +205,11 @@ defmodule Membrane.Element.Base.Mixin.CommonBehaviour do default_val_desc = if Keyword.has_key?(v, :default) do + default_formatter = + v |> Keyword.get(:default_formatter, quote(do: &"`#{inspect(&1)}`")) Backticks suggest that there's a term between them, while formatter may return kind of description of what the default value is, which may not be a term. For example '2 ms' returned by Membrane.Time.pretty_duration/1 shouldn't be considered a term to me. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBXhMZHes1dwxFzAeR8VsORdi0_g-uWks5u52otgaJpZM4ZUE7f> .

mat-hek · 2018-12-17T10:11:01Z

Backticks are not just for terms but for anything you want to present using monospace font. In docs I usually put values like this in backticks. In this case I would also use them to keep the style for different options cosistent. Anyway, It's a matter of opinion so we probably need a 3rd vote 😉

@bblaszkow06 I think that it's not really important whether the font is monospaced or not in this case, but it's important to mark terms so they could be easily distinguished from usual words

bblaszkow06 · 2018-12-19T15:08:22Z

lib/membrane/element/base/mixin/common_behaviour.ex

+    keyword: [spec: quote_expr(keyword)],
+    struct: [spec: quote_expr(struct)],
+    caps: [spec: quote_expr(struct)],
+    time: [spec: quote_expr(Time.t()), inspector: &Time.pretty_duration/1]


Although this pretty_duration is ok, I don't think it is a good way to present the default value. IMHO it should present the call to (Time.some_unit) because otherwise, it would be quite misleading

…pector

bblaszkow06 · 2018-12-20T12:07:15Z

lib/membrane/time.ex

@@ -50,6 +50,16 @@ defmodule Membrane.Time do
  @type non_neg_t :: non_neg_integer
  @type native_t :: integer

+  @units %{
+    days: :d,


Is there a reason why the units are atoms, not strings?

You need only one colon instead of a pair of quotes :D

bblaszkow06 · 2018-12-20T12:08:08Z

lib/membrane/time.ex

+      quote do 2 |> #{inspect(__MODULE__)}.nanoseconds() end |> Macro.to_string()
+
+  """
+  @spec pretty_duration(t) :: Macro.t()


copy paste went wrong :(

bblaszkow06 · 2018-12-20T12:13:40Z

lib/membrane/time.ex

@@ -50,6 +50,16 @@ defmodule Membrane.Time do
  @type non_neg_t :: non_neg_integer
  @type native_t :: integer

+  @units %{


This name clashes with units function. If the values here were strings I would name it time_unit_suffixes or sth like that. You may also consider some other name for units function.

added :default_formatter option parameter and Membrane.Time.pretty_du…

34b5eb6

…ration/1

mat-hek requested a review from bblaszkow06 December 14, 2018 19:02

bblaszkow06 requested changes Dec 15, 2018

View reviewed changes

mat-hek added 2 commits December 17, 2018 11:13

fixes according to review #134

d144b25

renamed default_formatter -> inspector, added time type

d722d61

bblaszkow06 self-requested a review December 19, 2018 14:56

bblaszkow06 reviewed Dec 19, 2018

View reviewed changes

added Time.to_code/1 and Time.to_code_str/1, used as default time ins…

98074c7

…pector

mat-hek requested a review from bblaszkow06 December 19, 2018 15:55

bblaszkow06 requested changes Dec 20, 2018

View reviewed changes

mat-hek added 2 commits December 21, 2018 14:52

small fixes and refactor in Membrane.Time

4608dc0

getting Bunch from github

45cb2a1

mat-hek requested review from bblaszkow06 and removed request for bblaszkow06 December 21, 2018 13:59

bblaszkow06 approved these changes Dec 28, 2018

View reviewed changes

mat-hek merged commit ce96116 into master Jan 4, 2019

bblaszkow06 deleted the feature/default-formatter branch February 15, 2019 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added :default_formatter option parameter and Membrane.Time.pretty_du… #134

added :default_formatter option parameter and Membrane.Time.pretty_du… #134

mat-hek commented Dec 14, 2018

bblaszkow06 Dec 15, 2018

bblaszkow06 Dec 15, 2018

bblaszkow06 Dec 15, 2018

mat-hek Dec 17, 2018

bblaszkow06 Dec 15, 2018

bblaszkow06 Dec 15, 2018

bblaszkow06 Dec 15, 2018

bblaszkow06 Dec 15, 2018

bblaszkow06 commented Dec 17, 2018 via email

mat-hek commented Dec 17, 2018 •

edited

Loading

bblaszkow06 Dec 19, 2018

bblaszkow06 Dec 20, 2018

mat-hek Dec 21, 2018

bblaszkow06 Dec 20, 2018

bblaszkow06 Dec 20, 2018


		## Examples

		iex> 10 \|> #{inspect(__MODULE__)}.milliseconds() \|> #{inspect(__MODULE__)}.pretty_duration()

added :default_formatter option parameter and Membrane.Time.pretty_du… #134

added :default_formatter option parameter and Membrane.Time.pretty_du… #134

Conversation

mat-hek commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bblaszkow06 commented Dec 17, 2018 via email

mat-hek commented Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat-hek commented Dec 17, 2018 •

edited

Loading