-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implementing few features #1
base: master
Are you sure you want to change the base?
Changes from 26 commits
ba18332
c3c6789
0907115
8334625
d7cd346
63a16bf
7f57601
61ab36b
693fc77
42a1bed
57923ba
47485ad
5313a2b
c464a02
e2cf5f7
d302482
0d61b1e
574d7a9
4d8c737
9bdc74a
87fae25
6887e97
f4abe43
9a4dc7f
ee27150
e598f80
da4e3fa
b3803e9
db8a83d
2a84eb7
265ae51
9c97664
33f8a68
2f1f279
c844aba
36f9176
b6aaa84
fa4e0da
6c0bbf1
ae8e0c0
1385cb3
4d7ccbf
ba76b77
e90b467
923a3b5
99c4dba
48ff9d0
ed96a02
ffca40c
b7a8427
592bdb1
445acb9
b371e98
2608cb3
ab21124
953499b
97be757
968bc6e
3264a8d
5d2f500
6c434d2
fdeaa40
7a3316b
0270d3c
edef4da
1fcb373
1393cc9
17540f4
1c8b801
a145b7e
8a9a80d
a4fd93d
7dfcff3
def8fde
84e40bc
762da75
ec064bf
6d3cb88
622947e
3dfb63d
75c974c
d31413d
6fa2978
0068f16
0ad2def
2dcc315
3383b23
dc103d3
4acc525
4a63d2a
3b5f6f1
1ca1c20
7a86668
b025f98
79adbb5
7baa611
a2fb44a
f8dc911
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,100 @@ | ||||||
defmodule Jake.Array do | ||||||
def gen(_) do | ||||||
StreamData.constant([]) | ||||||
@type_list [ | ||||||
%{"type" => "integer"}, | ||||||
%{"type" => "number"}, | ||||||
%{"type" => "boolean"}, | ||||||
%{"type" => "string"}, | ||||||
%{"type" => "null"}, | ||||||
nil | ||||||
] | ||||||
|
||||||
@min_items 0 | ||||||
|
||||||
@max_items 1000 | ||||||
|
||||||
def gen_array(map, type), do: arraytype(map, map["enum"], map["items"]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be we can name things by what they are about. E.g. Array could be about list or typle. The code seems to handle both cases, but probably the cases could be named better? E.g. Rename arraytype methods to either def gen_array(%{"items": items}=map, _) do
cond do
is_map(items) or items == nil -> list_gen(map, items) # note: not passing map["enum"], as mentioned in previous comment
is_array(items) -> tuple_gen(map, items)
_ -> raise error # ?
end
end
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree about naming things properly. It will avoid unnecessary ambiguity . |
||||||
|
||||||
def arraytype(map, enum, items) when enum != nil, do: Jake.gen_enum(enum, "array") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could this happen? won't the enum get handle at the Jake module? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider schema, If schema is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but the pattern match for enum happens early? so this will never get executed?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I'll move it lower. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw, is it really necessary to handle enum in all type-specific generator? anyway it's going to call the root module? we could instead just remove the enum handling from all the type-specific generators? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was also thinking the same. I'll check it out! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It won't handle case like I'll see, if it can be done without complicating things. |
||||||
|
||||||
def arraytype(map, enum, items) when is_list(items) do | ||||||
list = for n <- items, is_map(n), do: Jake.gen_init(n) | ||||||
|
||||||
{min, max} = get_min_max(map) | ||||||
|
||||||
case map["additionalItems"] do | ||||||
x when (is_boolean(x) and x) or is_nil(x) -> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about x when is_map(x) ->
generate_list(list, Jake.gen_init(map), max, min)
x when x == true or x == nil ->
generate_list(list, get_one_of(), max, min)
_ ->
unless length(list) in min..max, do: raise ...
StreamData.fixed_list(list) Avoid extra indirection? Otherwise it becomes difficult to understand the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is much better. I'll apply necessary changes. |
||||||
add_additional_items(list, true, max, min) | ||||||
|
||||||
x when is_map(x) -> | ||||||
add_additional_items(list, x, max, min) | ||||||
|
||||||
_ -> | ||||||
add_additional_items(list, false, max, min) | ||||||
end | ||||||
end | ||||||
|
||||||
def arraytype(map, enum, items) when is_map(items) do | ||||||
{min, max} = get_min_max(map) | ||||||
item = Jake.gen_init(items) | ||||||
decide_min_max(map, item, min, max) | ||||||
end | ||||||
|
||||||
def get_min_max(map) do | ||||||
min = Map.get(map, "minItems", @min_items) | ||||||
max = Map.get(map, "maxItems", @max_items) | ||||||
{min, max} | ||||||
end | ||||||
|
||||||
def arraytype(map, enum, items) when is_nil(items) and is_nil(enum) do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly could be combined with the previous def list_gen(map, items) when is_nil(items) or is_map(items) do
item = if is_nil(items), do: get_on_of(), else: Jake.gen_init(items)
{min, max} = get_min_max(map)
decide_min_max(map, item, min, max)
end |
||||||
item = get_one_of() | ||||||
{min, max} = get_min_max(map) | ||||||
decide_min_max(map, item, min, max) | ||||||
end | ||||||
|
||||||
def decide_min_max(map, item, min, max) | ||||||
when is_integer(min) and is_integer(max) and min < max do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if this doesn't match? If it should throw error, then being explicit about it might be better. |
||||||
if map["uniqueItems"] do | ||||||
StreamData.uniq_list_of(item, min_length: min, max_length: max) | ||||||
else | ||||||
StreamData.list_of(item, min_length: min, max_length: max) | ||||||
end | ||||||
end | ||||||
|
||||||
def get_one_of() do | ||||||
for(n <- @type_list, is_map(n), do: Jake.gen_init(n)) |> StreamData.one_of() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could this reuse the notype generator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. In notype, a type will be added to the schema depending on the properties. In get_one_of, it is not needed, since we already know type, here we using it to generate random values from random types, when no information is given about array. I think here, we can also use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
end | ||||||
|
||||||
def add_additional_items(list, bool, max, min) when is_boolean(bool) and bool do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this the same as pattern matching bool = true? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it is same as bool == true, but gives much better idea about code, since bool can be map also, which is handled by another overloaded function. |
||||||
generate_list(list, get_one_of(), max, min) | ||||||
end | ||||||
|
||||||
def add_additional_items(list, bool, max, min) when is_boolean(bool) and not bool do | ||||||
if length(list) in min..max do | ||||||
StreamData.fixed_list(list) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what will happen on else? could it return nil There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have list, but additional items are not allowed and length(list) (i.e. total items) do not fit in the range then test will fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have seen this pattern in multiple places, my question is why are returning nil when we know it's going to fail anyway. Assuming the expected return type is a generator, returning nil here would cause some random error in some unrelated place? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One way, I look at it is that it is easier to understand what code is doing even after few years.
Yes, it is possible. May be we can use try..catch block or raise some custom error to make things better. I think, nil can also be used as error code in this case, which means no generator, and this case needs to be handled by caller. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throwing error would be better. Nil being used as error is an assumption that would have to be remembered. Explicit is better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm also thinking the same. |
||||||
end | ||||||
end | ||||||
|
||||||
def add_additional_items(list, map, max, min) when is_map(map) do | ||||||
generate_list(list, Jake.gen_init(map), max, min) | ||||||
end | ||||||
|
||||||
def generate_list(olist, additional, max, min) do | ||||||
StreamData.bind(StreamData.fixed_list(olist), fn list -> | ||||||
StreamData.bind_filter( | ||||||
StreamData.list_of(additional), | ||||||
fn | ||||||
nlist | ||||||
when (length(list) + length(nlist)) in min..max -> | ||||||
{:cont, StreamData.constant(list ++ nlist)} | ||||||
|
||||||
nlist | ||||||
when length(list) in min..max -> | ||||||
{:cont, StreamData.constant(list)} | ||||||
|
||||||
nlist when true -> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Won't that be enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
certainly |
||||||
:skip | ||||||
end | ||||||
) | ||||||
end) | ||||||
end | ||||||
end |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
# Ref: https://stackoverflow.com/questions/38864001/elixir-how-to-deep-merge-maps | ||
# Ref: https://github.com/activesphere/jake/blob/master/lib/jake/map_util.ex | ||
|
||
defmodule Jake.MapUtil do | ||
def deep_merge(left, right) do | ||
Map.merge(left, right, &deep_resolve/3) | ||
end | ||
|
||
defp deep_resolve(_key, left, nil) do | ||
left | ||
end | ||
|
||
defp deep_resolve(_key, nil, right) do | ||
right | ||
end | ||
|
||
defp deep_resolve(key, left, right) when key == "type" do | ||
case {is_list(left), is_list(right)} do | ||
{x, y} when x and y -> left ++ right | ||
{x, y} when x and not y -> left ++ [right] | ||
{x, y} when not x and y -> [left] ++ right | ||
{x, y} when not x and not y -> [left, right] | ||
end | ||
end | ||
|
||
defp deep_resolve(_key, left, right) when is_map(left) do | ||
Map.merge(left, right) | ||
end | ||
|
||
defp deep_resolve(_key, left, right) when is_list(left) do | ||
left ++ right | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
defmodule Jake.Notype do | ||
@prop %{ | ||
"minLength" => "string", | ||
"maxLength" => "string", | ||
"pattern" => "string", | ||
"multipleOf" => "number", | ||
"minimum" => "number", | ||
"maximum" => "number", | ||
"exclusiveMinimum" => "number", | ||
"exclusiveMaximum" => "number", | ||
"items" => "array", | ||
"additionalItems" => "array", | ||
"minItems" => "array", | ||
"maxItems" => "array", | ||
"uniqueItems" => "array", | ||
"properties" => "object", | ||
"patternProperties" => "object", | ||
"additionalProperties" => "object", | ||
"dependencies" => "object", | ||
"required" => "object", | ||
"minProperties" => "object", | ||
"maxProperties" => "object" | ||
} | ||
|
||
def gen_notype(map, type) do | ||
nmap = for {k, v} <- map, into: %{}, do: {k, v} | ||
nlist = for {k, v} <- map, into: [], do: @prop[k] | ||
|
||
types = | ||
Enum.reduce(nlist, nil, fn | ||
x, acc when not is_nil(x) -> x | ||
x, acc when is_nil(x) -> acc | ||
end) | ||
|
||
nmap = if not is_nil(types), do: Map.put(nmap, "type", types), else: nmap | ||
if nmap["type"], do: Jake.gen_init(nmap) | ||
end | ||
end |
This file was deleted.
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.
why do we have to do all these? can't we just use member_of, assuming the given schema is valid?
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.
Consider the case when enum list for type number consists of null or string items along with regular numbers, then if we use only member_of without filtering first, then we may get wrong result, even if it is possible to get correct result.
You can also check out the test integer enum
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 had assumed enum value would be always correct, I don't understand why would anybody use invalid values.
But let's say we follow the spec and validate the value against enclosing spec, we only seem to handle the
type
right? what if it's a complex case which has other constraints likemaximum
etc? Do you think this could be handled in a simpler way?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.
In fact, I had also assumed the same, but later on looked at schema doc, which has mentioned that
the elements in the enum array should also be valid against the enclosing schema:
, therefore, I decided to filter the enum list depending on the schema type.yes
I also thought about it, but it is difficult and hence handled only simple case. I think, validating particular value against schema is out of scope of this project and is a separate project in itself. For this we can use something like
ExJsonSchema.Validator
, in the beginning, to filter out valid enum values, which then can feed to StreamData.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 is possibly a check on the sanity of the schema itself? It complicates the code here. Probably a separate tool could be used to warn/show error to user, if the schema has wrong enclosing type for enums given. This library could assume that the schema is correct. And only concentrate on generating data for correct schema.
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.
It will certainly simplify things.