Complete typespecs and guards for Module module #5869

eksperimental · 2017-03-13T15:07:59Z

There are plenty of changes, that's why the [WIP], but it all guards and typespecs are complete for this module.

There is a common argument which is the {<function/macro atom>, <arity>} tuple,
and it make me think whether we need to create a new type for this.

josevalim · 2017-03-13T15:47:00Z

lib/elixir/lib/module.ex

@@ -725,7 +733,7 @@ defmodule Module do
      end

  """
-  def defines?(module, tuple) when is_tuple(tuple) do
+  def defines?(module, tuple) when is_atom(module) and is_tuple(tuple) and tuple_size(tuple) == 2 do


Let's do defines?(module, {_, _} = tuple) when is_atom(module).

great!
besides being neater, does it perform faster?

perf diff is negligent.

do you mean negligible?

hahahahaha, yes

josevalim · 2017-03-13T15:47:02Z

lib/elixir/lib/module.ex

@@ -749,7 +757,9 @@ defmodule Module do
      end

  """
-  def defines?(module, tuple, kind) do
+  @spec defines?(module, {function_name :: atom, arity}, :def | :defp | :defmacro | :defmacrop) :: boolean


Let's do defines?(module, {_, _} = tuple, ...) when is_atom(module).

fixed and improved

josevalim · 2017-03-13T15:47:35Z

@eksperimental yup, a new type would be super helpful here.

eksperimental · 2017-03-13T16:29:53Z

@josevalim so what should we name it?

@type function_macro_name_arity :: {atom, arity} ?

josevalim · 2017-03-13T16:32:00Z

@eksperimental @type function_arity :: {function, arity}.

eksperimental · 2017-03-13T16:42:32Z

you mean atom not function, right?

josevalim · 2017-03-13T16:43:39Z

I meant function, it is an actual built-in type that is a synonym to atom.

eksperimental · 2017-03-13T17:02:08Z

@josevalim that is not documented in Elixir, and in Erlang
http://erlang.org/doc/reference_manual/typespec.html
function() is defined as fun().

fishcakez · 2017-03-13T17:12:21Z

@eksperimental IIRC it didn't used to be documented in Erlang and when it wasn't documented it meant atom(). Then some people pointed this out, and it got changed to fun() and documented.

josevalim · 2017-03-13T17:31:57Z

So let's go with atom() to avoid confusion.

eksperimental · 2017-03-13T17:35:00Z

thank you @fishcakez, interesting and ambiguous.
I am trying to see when this change happened in OTP 18 or later, to see if we can include it in the typespecs page

eksperimental · 2017-03-13T17:47:47Z

So it is documented at since OTP-17.4
erlang/otp@b7f72c8
So I guess we can document it in Elixir as well.
I will submit a PR.
thank you @fishcakez for the clarification

josevalim · 2017-03-16T08:54:19Z

Please let us know when this is ready for another pass.

eksperimental · 2017-03-16T11:49:34Z

sorry @josevalim,
this is ready for review since 3 days ago

fishcakez

There are a few spec changes, and I suggested a few spec changes for completeness.

I am not sure I am a fan of these PRs adding guards everywhere. If I was writing the code fresh I would not put them because I am not sure they add value and make the code unnecessarily verbose. Maybe I am missing something but it feels like busy work to me. Is there a issue/email/comment detailing the goal of doing this?

fishcakez · 2017-03-16T19:02:40Z

lib/elixir/lib/module.ex

@@ -1015,7 +1040,8 @@ defmodule Module do
  @doc false
  # Used internally to compile types.
  # This function is private and must be used only internally.
-  def store_typespec(module, key, value) when is_atom(key) do
+  @spec store_typespec(module, key :: atom, value :: term) :: Keyword.t


This function returns true, not Keyword.t.

Personally I think adding type checking guards to internal functions like this one is too defensive. Normally type check on boundary and use guards for flow control internally.

Is there reason spec'ed this undocumented function and not the other two (get/put attribute)?

Since this is @doc false, I agree. No need or specs nor guards.

spec removed

fishcakez · 2017-03-16T19:10:05Z

lib/elixir/lib/module.ex

@@ -463,7 +467,7 @@ defmodule Module do
  """
  def create(module, quoted, opts)

-  def create(module, quoted, %Macro.Env{} = env) do
+  def create(module, quoted, %Macro.Env{} = env) when is_atom(module) do


create/3 is missing spec

done. please review it

fishcakez · 2017-03-16T19:10:52Z

lib/elixir/lib/module.ex

@@ -550,18 +554,19 @@ defmodule Module do

  """
  @spec safe_concat(binary | atom, binary | atom) :: atom | no_return
-  def safe_concat(left, right) do


This should be :: atom, no no_return, could you fix this as you are fixing the other specs?

fishcakez · 2017-03-16T19:12:40Z

lib/elixir/lib/module.ex

@@ -788,7 +806,8 @@ defmodule Module do
      end

  """
-  def definitions_in(module, kind) do
+  @spec definitions_in(module, atom) :: [function_arity]


Is atom too general? Perhaps the different kinds could have their own types and reused throughout the specs?

yes. it's a good idea. done!

fishcakez · 2017-03-16T19:13:10Z

lib/elixir/lib/module.ex

@@ -801,14 +820,16 @@ defmodule Module do
  developer to customize it. See `Kernel.defoverridable/1` for
  more information and documentation.
  """
-  def make_overridable(module, tuples) do
+  @spec make_overridable(module, [function_arity]) :: :ok | no_return


Should be :: :ok, no no_return

There's still the no_return here 😕

fishcakez · 2017-03-16T19:14:57Z

lib/elixir/lib/module.ex

@@ -970,6 +994,7 @@ defmodule Module do
      ["Very", "Long", "Module", "Name", "And", "Even", "Longer"]

  """
+  @spec split(module) :: [String.t]


Will return non-empty list of strings: [String.t, ...]

fishcakez · 2017-03-16T21:48:43Z

So the guards will cause an immediate function clause error on bad arguments. This should help users fix bugs in their code without needing to read the source of elixir. It is especially to improve experience for new users who won't be familiar with debugging elixir or know where to look.

…

On 16 Mar 2017 12:03 pm, "José Valim" ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5869 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB6JTfgsn1lG48Tyys0lhAJ9CkfhNoWOks5rmST8gaJpZM4MbYaB> .

eksperimental · 2017-03-20T06:30:36Z

suggestions added, please review them.

whatyouhide · 2017-03-20T11:47:51Z

lib/elixir/lib/module.ex

+  @spec defines?(module, function_arity) :: boolean
+  def defines?(module, {function_macro_name, arity} = tuple)
+      when is_atom(module) and is_atom(function_macro_name)
+      and is_integer(arity) and arity >= 0 and arity <= 255 do


Please put the and at the end of the previous line, and indent is_integer to be aligned with the is_atom on the line above:

def ... when is_atom(module) and is_atom(function_macro_name) and is_integer(arity) and arity >= 0 ... do

whatyouhide · 2017-03-20T11:48:10Z

lib/elixir/lib/module.ex

+  @spec defines?(module, function_arity, def_kind) :: boolean
+  def defines?(module, {function_macro_name, arity} = tuple, def_kind)
+      when is_atom(module) and is_atom(function_macro_name)
+      and is_integer(arity) and arity >= 0 and arity <= 255


Same as https://github.com/elixir-lang/elixir/pull/5869/files#r106882072.

whatyouhide · 2017-03-20T11:48:53Z

lib/elixir/lib/module.ex

@@ -801,14 +820,16 @@ defmodule Module do
  developer to customize it. See `Kernel.defoverridable/1` for
  more information and documentation.
  """
-  def make_overridable(module, tuples) do
+  @spec make_overridable(module, [function_arity]) :: :ok | no_return


There's still the no_return here 😕

whatyouhide · 2017-03-20T11:51:08Z

I honestly feel a bit like @fishcakez, this PR is quite "heavy" on the verboseness side. I understand the point in doing this but I can't shake off the bad feeling

michalmuskala · 2017-03-20T13:46:20Z

To be honest, I'm not very keen on adding guards everywhere. They should be considered more closely, since they don't always improve errors.

For example with Enum.map where there was no function guard, when you passed wrong arity, you'd get BadArityError and now you get a generic FunctionClauseError - for me, the error is much worse now, the old one pointed at the issue much more directly.

josevalim · 2017-03-20T13:54:34Z

@michalmuskala we should revisit such cases then but I would say that's rather the exception than the rule. We should probably consider reverting the function guards there. Other cases where we typically raise good messages is when using maps.

Other than that, it is most likely that the error message will be bad. For example, an unchecked tuple can potentially raise badarg when calling element three or for function calls inside. That's arguably a worst experience.

For what is worth, most of the API in OTP is correctly guarded. In Elixir we never had the habit.

So overall the rationale is to provide better documentation and error messages. I agree the system is verbose and I don't like it either but if it improves the language experience with more documentation and error messages, then I think it is worth it.

eksperimental · 2017-03-20T16:29:51Z

@whatyouhide corrections made

whatyouhide · 2017-03-21T00:32:19Z

lib/elixir/lib/module.ex

        Module.definitions_in __MODULE__, :def  #=> [{:version, 0}]
-        Module.definitions_in __MODULE__, :defp #=> []
+        Module.definitions_in __MODULE__, :defmacro #=> []


Why this change? If you change version/0 to be a macro, then Module.definitions_in(__MODULE__, :def) will return [], not [{:version, 0}], and :defmacro will return [{:version, 0}] instead 😕

I was probably figuring out the spec and played with the example and accidentally saved it.

whatyouhide · 2017-03-21T00:32:34Z

lib/elixir/lib/module.ex

      end

  """
-  def definitions_in(module, kind) do
+  @spec definitions_in(module, def_kind) :: [function_arity]
+  def definitions_in(module, def_kind) when is_atom(module) and is_atom(def_kind) do


I think we should have the usual when def_kind in [:def, defp, ...] here.

whatyouhide · 2017-03-21T00:33:12Z

lib/elixir/lib/module.ex

-              Module.LocalsTracker.yank(module, tuple)
+    :lists.foreach(fn
+      {function_name, arity} = tuple
+          when is_atom(function_name) and is_integer(arity) and arity >= 0 and arity <= 255 ->


Please indent this on the same line as the lone above. It's not that long, but this indentation looks a bit off

do you mean move it to the line above, or indent it to the same level as the line above?

whatyouhide · 2017-03-21T00:33:39Z

lib/elixir/lib/module.ex

+                Module.LocalsTracker.yank(module, tuple)
+              end
+
+            old   = :elixir_overridable.overridable(module)


Can we drop the = alignment since we're touching these lines?

whatyouhide · 2017-03-21T00:34:46Z

lib/elixir/lib/module.ex

@@ -879,8 +910,8 @@ defmodule Module do
      end

  """
-  @spec get_attribute(module, atom) :: term
-  def get_attribute(module, key) do
+  @spec get_attribute(module, key :: atom) :: (value :: term)


I don't think we need to name the types here (key :: and value ::), they're clear from the function arguments and the function docs.

josevalim · 2017-03-21T09:59:54Z

lib/elixir/lib/module.ex

@@ -351,6 +351,10 @@ defmodule Module do
  the documentation for the [`:compile` module](http://www.erlang.org/doc/man/compile.html).
  '''

+  @type function_arity :: {atom, arity}


Let's make those typep as I don't want others to depend on it.

Changed function_arity, def_kind, type_kind to private.

josevalim · 2017-03-21T10:00:28Z

lib/elixir/lib/module.ex

-      kind in [:def, :defmacro, :type, :opaque] and (is_binary(doc) or is_boolean(doc) or doc == nil) do
+  def add_doc(module, line, kind, function_tuple, signature, doc)
+      when kind in [:def, :defmacro, :type, :opaque]
+      and (is_binary(doc) or is_boolean(doc) or doc == nil) do


And should be in the previous line and the parens should be aligned with the beginning of kind.

josevalim · 2017-03-21T10:01:39Z

lib/elixir/lib/module.ex

+  @spec defines?(module, function_arity, def_kind) :: boolean
+  def defines?(module, {function_macro_name, arity} = tuple, def_kind)
+      when is_atom(module) and is_atom(function_macro_name) and
+           is_integer(arity) and arity >= 0 and arity <= 255 and


Doesn't arity in 0..255 work here?

Nope. They wouldn't work at this point. Ranges work different than lists with in.

josevalim · 2017-03-21T10:02:16Z

lib/elixir/lib/module.ex

-      end
+      other ->
+        raise ArgumentError,
+              "each element in tuple list has to be a {function_name :: atom, arity :: 1..255} tuple, got: #{inspect(other)}"


Arity is from 0 to 255.

thank you. fixed

josevalim · 2017-03-21T10:02:26Z

lib/elixir/lib/module.ex

-  def overridable?(module, tuple) do
+  @spec overridable?(module, function_arity) :: boolean
+  def overridable?(module, {function_name, arity} = tuple)
+      when is_atom(function_name) and is_integer(arity) and arity >= 0 and arity <= 255 do


arity in 0..255?

It won't work.

josevalim · 2017-03-21T10:02:43Z

lib/elixir/lib/module.ex

@@ -879,8 +910,8 @@ defmodule Module do
      end

  """
-  @spec get_attribute(module, atom) :: term
-  def get_attribute(module, key) do
+  @spec get_attribute(module, atom) :: (term)


Please remove parens around term.

josevalim · 2017-03-21T10:02:52Z

lib/elixir/lib/module.ex

@@ -897,8 +928,8 @@ defmodule Module do
      end

  """
-  @spec delete_attribute(module, key :: atom) :: (value :: term)
-  def delete_attribute(module, key) when is_atom(key) do
+  @spec delete_attribute(module, atom) :: (term)


Please remove parens around term.

josevalim · 2017-03-21T10:03:11Z

lib/elixir/lib/module.ex

@@ -944,18 +975,20 @@ defmodule Module do
      end

  """
-  def register_attribute(module, new, opts) when is_atom(new) do
+  @spec register_attribute(module, attribute :: atom, opts :: [{:accumulate, boolean}, {:persist, boolean}])
+      :: :ok | no_return


Remove no_return from here per the previous discussions.

josevalim · 2017-03-21T10:04:54Z

lib/elixir/test/elixir/module_test.exs

@@ -310,4 +316,33 @@ defmodule ModuleTest do
      assert Module.definitions_in(__MODULE__, :defp) == []
    end
  end
+
+  describe "make_overridable/2 arguments" do


Check lib/elixir/test/elixir/kernel/overridable_test.exs. We don't need to add the success test because there are many there. You can add a test for the error in make_overridable though.

Thank you. have a look please.

eksperimental · 2017-03-21T16:01:01Z

@josevalim All your suggestions have been implemented

josevalim · 2017-03-21T16:08:41Z

Sorry but I can't see why the range version wouldn't work. arity in 0..255 expands to exactly the same code you wrote. -- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

eksperimental · 2017-03-21T16:19:03Z

Well, I originally did it that way and I had to change it because it wouldn't work.

(second line is the offending line no. 743)

  def defines?(module, {function_macro_name, arity} = tuple)
      when is_atom(module) and is_atom(function_macro_name) and arity in 0..255 do
    assert_not_compiled!(:defines?, module)
    table = defs_table_for(module)
    :ets.lookup(table, {:def, tuple}) != []
  end

$ make clean test will error:

...
==> bootstrap (compile)
Compiled lib/elixir/lib/kernel.ex
Compiled lib/elixir/lib/macro/env.ex
Compiled lib/elixir/lib/keyword.ex
error: undef
stacktrace: [{'Elixir.Macro',to_string,[{'..',[{line,743}],[0,255]}],[]},
             {'Elixir.Kernel',in,2,[{file,"expanding macro"}]},
             {'Elixir.Module','defines?',2,
                              [{file,"/home/eksperimental/elixir/lib/elixir/lib/module.ex"},
                               {line,743}]},
             {'Elixir.Kernel','and',2,[{file,"expanding macro"}]},
             {'Elixir.Module','defines?',2,
                              [{file,"/home/eksperimental/elixir/lib/elixir/lib/module.ex"},
                               {line,743}]},
             {elixir,eval_forms,4,[{file,"src/elixir.erl"},{line,216}]},
             {elixir_compiler,eval_forms,3,
                              [{file,"src/elixir_compiler.erl"},{line,61}]},
             {elixir_compiler,quoted,3,
                              [{file,"src/elixir_compiler.erl"},{line,27}]}]
Makefile:78: recipe for target 'lib/elixir/ebin/Elixir.Kernel.beam' failed

josevalim · 2017-03-21T16:31:19Z

Ah, that's because of bootstrapping. Ok. Let's go as is.

lexmag · 2017-03-22T00:51:05Z

lib/elixir/lib/module.ex

-  should be omitted for types) and the documentation, which should
-  be either a binary or a boolean.
+  It expects the module the function/type belongs to, the line (a non-negative integer),
+  the kind (`:def`, `defmacro`, `:type`, `:opaque`), a tuple `{<function atom>, <arity>}`,


:defmacro

function atom reads oddly, I think we can say function name instead.

lexmag · 2017-03-22T00:58:41Z

lib/elixir/lib/module.ex

@@ -351,6 +351,10 @@ defmodule Module do
  the documentation for the [`:compile` module](http://www.erlang.org/doc/man/compile.html).
  '''

+  @typep function_arity :: {atom, arity}


Maybe function_arity_pair?

I think we can name it definition, as it this name is already used in definitions_in function, which returns in exactly that format.

lexmag · 2017-03-22T01:01:01Z

lib/elixir/lib/module.ex

@@ -725,7 +738,10 @@ defmodule Module do
      end

  """
-  def defines?(module, tuple) when is_tuple(tuple) do
+  @spec defines?(module, function_arity) :: boolean
+  def defines?(module, {function_macro_name, arity} = tuple)


function_macro_name seems quite confusing to me, can we just say function_name?

Indeed, we use function_name in same context in make_overridable, and overridable?.

I am worried that function_name doesn't take macros. That's why the difference.
We could name it function_or_macro_name

👍 for clarity, let's go with function_or_macro_name :)

lexmag · 2017-03-22T01:02:09Z

lib/elixir/lib/module.ex

    assert_not_compiled!(:defines?, module)
    table = defs_table_for(module)
    case :ets.lookup(table, {:def, tuple}) do
-      [{_, ^kind, _, _, _, _}] -> true
+      [{_, ^def_kind, _, _, _, _}] -> true


Perhaps we should keep kind, we didn't change that in add_doc as well.

this is done intentionally, to be inlined with the typeps:
in add_doc is called kind because its typespec is a union def_kind | type_kind

lexmag · 2017-03-22T01:03:23Z

lib/elixir/lib/module.ex

    assert_not_compiled!(:definitions_in, module)
    table = defs_table_for(module)
-    :lists.concat :ets.match(table, {{:def, :'$1'}, kind, :_, :_, :_, :_})
+    :lists.concat :ets.match(table, {{:def, :'$1'}, def_kind, :_, :_, :_, :_})


Same as https://github.com/elixir-lang/elixir/pull/5869/files#r107312839.

lexmag · 2017-03-22T01:05:14Z

lib/elixir/lib/module.ex

+                Module.LocalsTracker.yank(module, tuple)
+              end
+
+            old  = :elixir_overridable.overridable(module)


Extra space here to ✂️.

lexmag · 2017-03-22T01:06:59Z

lib/elixir/test/elixir/module_test.exs

@@ -310,4 +316,19 @@ defmodule ModuleTest do
      assert Module.definitions_in(__MODULE__, :defp) == []
    end
  end
+
+  test "make_overridable/2 with bad arguments" do


I think we usually use invalid instead of bad.

lexmag · 2017-03-22T01:09:13Z

lib/elixir/test/elixir/module_test.exs

@@ -50,11 +50,17 @@ defmodule ModuleTest do
  end
  Module.eval_quoted __MODULE__, contents, [], file: "sample.ex", line: 13

+  defp purge(modules) do
+    Enum.each modules, fn(m) ->


Seems premature use of Enum here, in places where we call purge/1 helper we pass only one module.

lexmag · 2017-03-22T01:12:23Z

lib/elixir/lib/module.ex

-  def register_attribute(module, new, opts) when is_atom(new) do
+  @spec register_attribute(module, attribute :: atom, opts :: [{:accumulate, boolean}, {:persist, boolean}])
+      :: :ok
+  def register_attribute(module, attribute, opts) when is_atom(module) and is_atom(attribute) do


I'm wondering if we're fine to rename opts –> options?

eksperimental · 2017-03-22T05:17:02Z

@lexmag I have added a commit with your suggestions. I didn't add implement since I have left a comment waiting for your answer.

eksperimental · 2017-03-24T15:49:24Z

this is ready to merge.

whatyouhide · 2017-03-24T18:00:54Z

Thanks @eksperimental! 💟

eksperimental · 2017-03-25T04:54:41Z

thank you everyone (@elixir-lang/elixir) for your suggestions!

eksperimental force-pushed the module_typespecs branch from 67e7dfe to 0f70502 Compare March 13, 2017 15:45

josevalim reviewed Mar 13, 2017

View reviewed changes

eksperimental force-pushed the module_typespecs branch from 4f15863 to e543c8e Compare March 13, 2017 16:25

eksperimental changed the title ~~[WIP] Complete typespecs and guards for Module module~~ Complete typespecs and guards for Module module Mar 16, 2017

josevalim approved these changes Mar 16, 2017

View reviewed changes

fishcakez suggested changes Mar 16, 2017

View reviewed changes

josevalim approved these changes Mar 20, 2017

View reviewed changes

whatyouhide reviewed Mar 20, 2017

View reviewed changes

eksperimental force-pushed the module_typespecs branch from c0e97f7 to 8f6044f Compare March 20, 2017 16:12

eksperimental added 2 commits March 21, 2017 04:30

Complete typespecs and guards for Module module

f6d1cdd

Improve tuple pattern matching in Module functions

6a49786

eksperimental force-pushed the module_typespecs branch from e09f344 to 325ba79 Compare March 20, 2017 23:32

whatyouhide reviewed Mar 21, 2017

View reviewed changes

More suggestions

189f4c5

eksperimental force-pushed the module_typespecs branch from 35ad2a2 to 189f4c5 Compare March 21, 2017 02:38

josevalim reviewed Mar 21, 2017

View reviewed changes

More suggestions by José Valim

36604b7

lexmag suggested changes Mar 22, 2017

View reviewed changes

Suggestions by @lexmag

7db25d7

kelvinst approved these changes Mar 23, 2017

View reviewed changes

Rename argument function_macro_name to function_or_macro_name

ae54254

josevalim approved these changes Mar 24, 2017

View reviewed changes

whatyouhide merged commit eb0f3ea into elixir-lang:master Mar 24, 2017

eksperimental deleted the module_typespecs branch March 24, 2017 18:13

Complete typespecs and guards for Module module #5869

Complete typespecs and guards for Module module #5869

Conversation

eksperimental commented Mar 13, 2017

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

josevalim commented Mar 13, 2017

eksperimental commented Mar 13, 2017

josevalim commented Mar 13, 2017

eksperimental commented Mar 13, 2017

josevalim commented Mar 13, 2017

eksperimental commented Mar 13, 2017

fishcakez commented Mar 13, 2017

josevalim commented Mar 13, 2017

eksperimental commented Mar 13, 2017

eksperimental commented Mar 13, 2017

josevalim commented Mar 16, 2017

eksperimental commented Mar 16, 2017

fishcakez left a 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

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

fishcakez commented Mar 16, 2017 via email

eksperimental commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whatyouhide commented Mar 20, 2017

michalmuskala commented Mar 20, 2017 • edited

josevalim commented Mar 20, 2017

eksperimental commented Mar 20, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eksperimental commented Mar 21, 2017

josevalim commented Mar 21, 2017 via email

eksperimental commented Mar 21, 2017

josevalim commented Mar 21, 2017 via email • edited

Choose a reason for hiding this comment

michalmuskala commented Mar 20, 2017 •

edited

josevalim commented Mar 21, 2017 via email •

edited