From c83060f13c9dd5a189c1672f56095ad36790c90c Mon Sep 17 00:00:00 2001 From: Dave Lucia Date: Thu, 6 May 2021 07:44:37 -0400 Subject: [PATCH 1/3] Warn on invalid default values --- lib/surface/api.ex | 46 +++++++++++++++++++++++++++++--- test/api_test.exs | 4 +-- test/properties_test.exs | 57 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 5 deletions(-) diff --git a/lib/surface/api.ex b/lib/surface/api.ex index e9690fee..2d7a2e4b 100644 --- a/lib/surface/api.ex +++ b/lib/surface/api.ex @@ -371,11 +371,11 @@ defmodule Surface.API do end defp get_valid_opts(:prop, _type, _opts) do - [:required, :default, :values, :accumulate] + [:required, :default, :values, :values!, :accumulate] end defp get_valid_opts(:data, _type, _opts) do - [:default, :values] + [:default, :values, :values!] end defp get_valid_opts(:slot, _type, _opts) do @@ -408,7 +408,7 @@ defmodule Surface.API do {:error, "invalid value for option :required. Expected a boolean, got: #{inspect(value)}"} end - defp validate_opt(:prop, _name, _type, opts, :default, _value, caller) do + defp validate_opt(:prop, name, _type, opts, :default, value, caller) do if Keyword.get(opts, :required, false) do IOHelper.warn( "setting a default value on a required prop has no effect. Either set the default value or set the prop as required, but not both.", @@ -417,6 +417,14 @@ defmodule Surface.API do ) end + warn_on_invalid_default(:prop, name, value, opts, caller) + + :ok + end + + defp validate_opt(:data, name, _type, opts, :default, value, caller) do + warn_on_invalid_default(:data, name, value, opts, caller) + :ok end @@ -473,6 +481,38 @@ defmodule Surface.API do :ok end + defp warn_on_invalid_default(type, name, default, opts, caller) do + accumulate? = Keyword.get(opts, :accumulate, false) + values! = Keyword.get(opts, :values!) + + cond do + accumulate? and not is_list(default) -> + IOHelper.warn( + "#{type} `#{name}` default value #{inspect(default)} must be a list when accumulate: true", + caller, + fn _ -> caller.line end + ) + + accumulate? and not is_nil(values!) and + not MapSet.subset?(MapSet.new(default), MapSet.new(values!)) -> + IOHelper.warn( + "#{type} `#{name}` default value #{inspect(default)} does not exist in :values!", + caller, + fn _ -> caller.line end + ) + + not accumulate? and not is_nil(values!) and not (default in values!) -> + IOHelper.warn( + "#{type} `#{name}` default value #{inspect(default)} does not exist in :values!", + caller, + fn _ -> caller.line end + ) + + true -> + :ok + end + end + defp unknown_options_message(valid_opts, unknown_options) do {plural, unknown_items} = case unknown_options do diff --git a/test/api_test.exs b/test/api_test.exs index 4f726c8d..70f29918 100644 --- a/test/api_test.exs +++ b/test/api_test.exs @@ -289,7 +289,7 @@ defmodule Surface.APITest do code = "prop label, :string, a: 1" message = - ~r/unknown option :a. Available options: \[:required, :default, :values, :accumulate\]/ + ~r/unknown option :a. Available options: \[:required, :default, :values, :values!, :accumulate\]/ assert_raise(CompileError, message, fn -> eval(code) @@ -404,7 +404,7 @@ defmodule Surface.APITest do test "validate unknown type options" do code = "data label, :string, a: 1" - message = ~r/unknown option :a. Available options: \[:default, :values\]/ + message = ~r/unknown option :a. Available options: \[:default, :values, :values!\]/ assert_raise(CompileError, message, fn -> eval(code) diff --git a/test/properties_test.exs b/test/properties_test.exs index effce621..8d31f7b2 100644 --- a/test/properties_test.exs +++ b/test/properties_test.exs @@ -706,4 +706,61 @@ defmodule Surface.PropertiesSyncTest do code.exs:7:\ """ end + + test "warn if given default value doesn't exist in values list" do + id = :erlang.unique_integer([:positive]) |> to_string() + module = "TestComponentWithDefaultValueThatDoesntExistInValues_#{id}" + + code = """ + defmodule #{module} do + use Surface.Component + + prop type, :string, values!: ["small", "medium", "large"], default: "x-large" + + data data_type, :string, values!: ["small", "medium", "large"], default: "x-large" + + prop invalid_type, :integer, default: [], values!: [0, 1, 2] + + prop valid_acc, :integer, default: [1], values!: [0, 1, 2], accumulate: true + + prop invalid_acc1, :integer, default: [3], values!: [0, 1, 2], accumulate: true + + prop invalid_acc2, :string, values!: [1, 2, 3], default: 3, accumulate: true + + def render(assigns) do + ~H"" + end + end + """ + + output = + capture_io(:standard_error, fn -> + {{:module, _, _, _}, _} = + Code.eval_string(code, [], %{__ENV__ | file: "code.exs", line: 1}) + end) + + assert output =~ ~S""" + prop `type` default value "x-large" does not exist in :values! + """ + + assert output =~ ~S""" + data `data_type` default value "x-large" does not exist in :values! + """ + + assert output =~ ~S""" + prop `invalid_type` default value [] does not exist in :values! + """ + + refute output =~ ~S""" + prop `valid_acc` + """ + + assert output =~ ~S""" + prop `invalid_acc1` default value [3] does not exist in :values! + """ + + assert output =~ ~S""" + prop `invalid_acc2` default value 3 must be a list when accumulate: true + """ + end end From cc1cc291154aedd5ebcaaa5e84fe6a30049834d9 Mon Sep 17 00:00:00 2001 From: Dave Lucia Date: Fri, 7 May 2021 08:11:12 -0400 Subject: [PATCH 2/3] formatting of warnings --- lib/surface/api.ex | 6 +++--- test/properties_test.exs | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/surface/api.ex b/lib/surface/api.ex index 2d7a2e4b..a3aacba5 100644 --- a/lib/surface/api.ex +++ b/lib/surface/api.ex @@ -488,7 +488,7 @@ defmodule Surface.API do cond do accumulate? and not is_list(default) -> IOHelper.warn( - "#{type} `#{name}` default value #{inspect(default)} must be a list when accumulate: true", + "#{type} `#{name}` default value `#{inspect(default)}` must be a list when `accumulate: true`", caller, fn _ -> caller.line end ) @@ -496,14 +496,14 @@ defmodule Surface.API do accumulate? and not is_nil(values!) and not MapSet.subset?(MapSet.new(default), MapSet.new(values!)) -> IOHelper.warn( - "#{type} `#{name}` default value #{inspect(default)} does not exist in :values!", + "#{type} `#{name}` default value `#{inspect(default)}` does not exist in `:values!`", caller, fn _ -> caller.line end ) not accumulate? and not is_nil(values!) and not (default in values!) -> IOHelper.warn( - "#{type} `#{name}` default value #{inspect(default)} does not exist in :values!", + "#{type} `#{name}` default value `#{inspect(default)}` does not exist in `:values!`", caller, fn _ -> caller.line end ) diff --git a/test/properties_test.exs b/test/properties_test.exs index 8d31f7b2..2351bacb 100644 --- a/test/properties_test.exs +++ b/test/properties_test.exs @@ -740,15 +740,15 @@ defmodule Surface.PropertiesSyncTest do end) assert output =~ ~S""" - prop `type` default value "x-large" does not exist in :values! + prop `type` default value `"x-large"` does not exist in `:values!` """ assert output =~ ~S""" - data `data_type` default value "x-large" does not exist in :values! + data `data_type` default value `"x-large"` does not exist in `:values!` """ assert output =~ ~S""" - prop `invalid_type` default value [] does not exist in :values! + prop `invalid_type` default value `[]` does not exist in `:values!` """ refute output =~ ~S""" @@ -756,11 +756,11 @@ defmodule Surface.PropertiesSyncTest do """ assert output =~ ~S""" - prop `invalid_acc1` default value [3] does not exist in :values! + prop `invalid_acc1` default value `[3]` does not exist in `:values!` """ assert output =~ ~S""" - prop `invalid_acc2` default value 3 must be a list when accumulate: true + prop `invalid_acc2` default value `3` must be a list when `accumulate: true` """ end end From b98230cda00cc654ae1ba6814db9fb842e30e4f2 Mon Sep 17 00:00:00 2001 From: Dave Lucia Date: Mon, 10 May 2021 08:11:57 -0400 Subject: [PATCH 3/3] hint --- lib/surface/api.ex | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/surface/api.ex b/lib/surface/api.ex index a3aacba5..9dd3e0af 100644 --- a/lib/surface/api.ex +++ b/lib/surface/api.ex @@ -496,14 +496,22 @@ defmodule Surface.API do accumulate? and not is_nil(values!) and not MapSet.subset?(MapSet.new(default), MapSet.new(values!)) -> IOHelper.warn( - "#{type} `#{name}` default value `#{inspect(default)}` does not exist in `:values!`", + """ + #{type} `#{name}` default value `#{inspect(default)}` does not exist in `:values!` + + Hint: Either choose an existing value or replace `:values!` with `:values` to skip validation. + """, caller, fn _ -> caller.line end ) not accumulate? and not is_nil(values!) and not (default in values!) -> IOHelper.warn( - "#{type} `#{name}` default value `#{inspect(default)}` does not exist in `:values!`", + """ + #{type} `#{name}` default value `#{inspect(default)}` does not exist in `:values!` + + Hint: Either choose an existing value or replace `:values!` with `:values` to skip validation. + """, caller, fn _ -> caller.line end )