Support of nesting a record into a record #56
Conversation
Totktonada
commented
Feb 26, 2018
•
Totktonada
commented
- Related to Test record within a record #46.
- Fixes Avro schema with a record as a record field causes an error #49.
graphql/tarantool_graphql.lua
Outdated
| --- | ||
| --- @tparam table state for read state.accessor and previously filled | ||
| --- state.types | ||
| --- @tparam table fields fields part from an avro-schema | ||
| --- | ||
| --- @treturn table res XXX |
There was a problem hiding this comment.
Does 'XXX' mean obvious? Or it is TODO?
In case it is TODO, I would like to see there 'todo' instead of 'xxx', because first option do not require any explanations.
There was a problem hiding this comment.
XXX is like TODO, but is not commitment. It means 'smth is not good here'. When I add that to the ldoc comment it means 'I’ll fix it before considering the PR as ready'. XXX is standard mark for such things.
$ echo $((`ag -c '\bXXX\b' /usr/src/linux | cut -d: -f2 | tr '\n' '+'; echo 0`))
3182
$ echo $((`ag -c '\bTODO\b' /usr/src/linux | cut -d: -f2 | tr '\n' '+'; echo 0`))
4703
| shard.init(config) | ||
| end) | ||
| initialized = true | ||
| end |
There was a problem hiding this comment.
Can you explain, please?
There was a problem hiding this comment.
- I added the 2nd test. When the default (master) instance stops (the suite has
core = app) and starts from a snap/xlog from the previous test, the box.once will not invoke their function, so shard become uninitialized. - Not it works, but is not correct. We avoid the another problem: the shard module cannot be initialized twice, so we omit the 2nd initialization and proceed twice with the same (the first one) configuration.
There was a problem hiding this comment.
I updated the code with the comment.
There was a problem hiding this comment.
Filed the following issue for this problem: tarantool/graphql#57
| } | ||
| }]]), | ||
| service_fields = { | ||
| user = {}, |
There was a problem hiding this comment.
what does user = {}, mean? Is it neccessary?
There was a problem hiding this comment.
Empty list of service fields. Don’t sure we allow to skip it, but that has sense, yep.
graphql/tarantool_graphql.lua
Outdated
| @@ -149,27 +149,30 @@ local function gql_argument_type(state, avro_schema) | |||
| end | |||
| end | |||
|
|
|||
| local function convert_record_fields_to_args(state, fields) | |||
| local function convert_record_fields_to_args(state, fields, skip_compound) | |||
There was a problem hiding this comment.
I prefer skip_compound -> skip_nested
There was a problem hiding this comment.
Scalar fields are nested too.
Totktonada
force-pushed
the
tkn/gh-46-nested-record
branch
from
310d378 to
62f9673
Compare
Totktonada
changed the title
Totktonada
force-pushed
the
tkn/gh-46-nested-record
branch
from
62f9673 to
050ee8e
Compare
graphql/tarantool_graphql.lua
Outdated
| --- Convert each field of an avro-schema to a scalar graphql type or an input | ||
| --- object. | ||
| --- | ||
| --- @tparam table fields list of fields the avro-schema record fields format |
There was a problem hiding this comment.
I believe you forgot "in"
graphql/tarantool_graphql.lua
Outdated
| --- | ||
| --- @tparam[opt] table opts optional options: | ||
| --- | ||
| --- * skip_compound -- do not add fields of record type to the arguments; |
There was a problem hiding this comment.
I try to wrap things like skip_compound into back quotes. However I im not sure is it is a good practice
There was a problem hiding this comment.
I like such visual accents too.
graphql/tarantool_graphql.lua
Outdated
| --- * skip_compound -- do not add fields of record type to the arguments; | ||
| --- default: false. | ||
| --- | ||
| --- @treturn table `args` -- map with type names as keys and graphql types as |
There was a problem hiding this comment.
Why did you wrap args in ` while in other similar cases you do not to this?
There was a problem hiding this comment.
@tparam marks the parameter name monotyped automatically, but @treturn is not.
| args[field.name] = nullable(gql_class) | ||
| if not skip_compound or avro_type(field.type) ~= 'record' then | ||
| local gql_class = gql_argument_type(field.type) | ||
| args[field.name] = nullable(gql_class) |
There was a problem hiding this comment.
Why do you call nullable without any conditions?
There was a problem hiding this comment.
The idea was that you can omit any argument. But AFAIR it is so even if you don’t use 'nullable'.
Totktonada
force-pushed
the
tkn/gh-46-nested-record
branch
from
050ee8e to
efc542f
Compare
Totktonada
force-pushed
the
tkn/gh-46-nested-record
branch
from
efc542f to
2eecc27
Compare
