-
Notifications
You must be signed in to change notification settings - Fork 13
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
Apib attributes #44
Apib attributes #44
Conversation
lib/blue_bird/json_data.ex
Outdated
iex> JSONData.type(1) | ||
"number" | ||
""" | ||
@spec type(Any) :: String.t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be type(any)
.
Apart from that, I'm getting these dialyzer warnings:
:0: Unknown function 'Elixir.BlueBird.JSONData.Function':'__impl__'/1
:0: Unknown function 'Elixir.BlueBird.JSONData.PID':'__impl__'/1
:0: Unknown function 'Elixir.BlueBird.JSONData.Port':'__impl__'/1
:0: Unknown function 'Elixir.BlueBird.JSONData.Reference':'__impl__'/1
I tried setting @fallback_to_any true
, but that didn't help. So either we have to add useless implementations for Function, PID, Port, and Reference, or we have to configure Dialyxir to ignore these warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I will fix the typo.
Regardless the Unknown function
I would prefer to add the missing implementations. The return value should be null
.
lib/blue_bird/writer/blueprint.ex
Outdated
defp print_attribute_name_and_type(key, value), do: "+ #{key} (#{JSONData.type(value)})\n" | ||
|
||
defp walk_through_attributes(attributes, indent \\ 8) do | ||
(attributes |> Enum.map_join(&(print_attribute(&1))) |> indent(indent)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many brackets.
lib/blue_bird/writer/blueprint.ex
Outdated
print_attribute_name_and_type(key, value) | ||
<> walk_through_attributes(value, 4) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many line breaks (here and after the function).
lib/blue_bird/writer/blueprint.ex
Outdated
end | ||
end | ||
|
||
defp print_attribute({key, value}), do: print_attribute_name_and_type(key, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too long lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check the credo configuration? It doesn't emit a warning here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there's a credo warning for generator.ex
line 176, which you didn't change, so I wonder why it went through Travis without notice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have any credo warnings at all.... I'm always using credo with --strict
...
test/generator_test.exs
Outdated
@@ -230,7 +256,31 @@ defmodule BlueBird.Test.GeneratorTest do | |||
|
|||
assert request.query_params == %{"s" => "poodle"} | |||
assert request.path_params == %{"id" => "137"} | |||
assert request.body_params == %{"betty" => "white"} | |||
assert request.body_params == %{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we gain from this? The generator only sets request.body_params
to conn.body_params
. So we only have to test whether the field was set as expected, but we don't have to test whether Plug is able to decode complex json objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, you might be right... this test is maybe not that useful. In fact the function itself is tested and it is not necessary anymore to do that here. I will change that.
@@ -1,4 +1,4 @@ | |||
defmodule BlueBird.Test.Support.TestControllerUndocumented do | |||
defmodule BlueBird.Test.Support.TestUndocumentedController do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BlueBird.Test.Support.TestController
, BlueBird.Test.Support.TestNamedController
and BlueBird.Test.Support.TestUndocumentedController
is a bit redundant. Can we rename them to BlueBird.Test.Support.Controller
etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... sure.
test/writer/blueprint_test.exs
Outdated
end | ||
|
||
test "prints attributes correctly" do | ||
assert print_attributes(@complex_body) == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put the first """
in the assert-line, and then indent the block beginning with + Attributes
with two spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. But I will change it then on every other place, too. Consistency, you know. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
lib/blue_bird/writer/blueprint.ex
Outdated
<> walk_through_attributes(attributes) | ||
end | ||
|
||
@spec print_attribute(Tuple.t) :: String.t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be @spec print_attribute({String.t, any}) :: String.t
.
No description provided.