Split-up modules and refactor arguments converting #168

Totktonada · 2018-06-06T10:49:12Z

Fixes #166.

Part of #166.

Fixed ldoc comment.

It is to make module name consistent with the variable name by which it is used.

Moves are mostly done w/o changes. There are APIs (parameters and return values) that are subject to further refactoring. Removed duplicate code in resolve the function for multihead connections. Part of #166.

* Generate object arguments in avro-schema format first. * Generate object arguments for subrecords. * Use full path for a GraphQL type name where possible. Also: * Added apidoc-lint target and added to CI. * Part of #166. * Fixes #73 (2nd bullet). * Fixes #163. * Fixes #46.

Fixes #166.

Part of #166.

SudoBobo · 2018-06-14T08:36:23Z

graphql/convert_schema/arguments.lua

+    local context = opts.context
+
+    check(type_name, 'type_name', 'string', 'nil')
+    check(context, 'context', 'table')


Seems a little bit strange. Context is in opts, but it is mandatory argument. Why not make a separate argument? (Maybe I missed something about our concept of opts)

Maybe I’ll make it optional sooner or later, expose convert_schema.arguments.convert as public function and remove arguments.convert_record_fields.

SudoBobo · 2018-06-14T08:42:44Z

graphql/convert_schema/arguments.lua

+        if type(avro_schema.name) ~= 'string' then -- avoid extra json.encode()
+            assert(type(avro_schema.name) == 'string',
+                ('avro_schema.name must be a string, got %s (avro_schema %s)')
+                :format(type(avro_schema.name), json.encode(avro_schema)))


Maybe remove assert(sure_to_be_false_condition, message) with error(message) ?

It would be good to use error / assert according to some specific rules (@Khatskevich had a proposal on the topic, AFAIR?). Now it is arbitrary with bias toward assert.

SudoBobo · 2018-06-14T10:22:48Z

graphql/convert_schema/arguments.lua

+        local avro_t = avro_helpers.avro_type(field.type)
+        local type_name
+        if (avro_t == 'record' or avro_t == 'record*') and
+                field.type.name:startswith(root_name) then


Why 2 tabs instead of one?

To visually distinguish if-condition from if-branch-body. It seems not being most popular indent style, but I have to use it.

But it's all in if-condition, I mean, both lines

<0 indent>if <cond start> <2 indent><cond middle> <2 indent><cond end> then <1 indent><if branch body> <0 indent>end

SudoBobo · 2018-06-14T10:41:08Z

graphql/convert_schema/arguments.lua

+            type_name = type_name,
+        })
+        context.field_name = nil
+        table.remove(context.path, #context.path)


Do we really need to use this mutable context variable? What do you think about passing (full) path and a field name strings into convert()? It seems to work the same way as with this table.remove(context.path,context.path) If I am not clear, we can discuss it verbaly

It just seems a little bit overcomplicated to use explicit stack where we can just pass arguments

My intension was hide details of the context inside a variable and use/change needed things only where it is needed. It allows to avoid formal arguments that just pass-through them to call of an another function.

SudoBobo · 2018-06-14T11:00:37Z

graphql/impl.lua

@@ -0,0 +1,297 @@
+--- Implementation of module-level functions and functions of instances of the
+--- graphql library and a compiled query.
+


Are you sure about naming? "Implementation" sounds quite wide and uncertain, maybe something like "Instance" and think about as a set of interface functions, connecting user and inner mechanisms of tarantool graphql?

Instance was the first variant I consider about :) I was confused with the fact that require('graphql.instance') would return something that is not an instance.

Now I think about it as *.c (impl.lua) and *.h (init.lua) files. Maybe I should rename it to init_impl.lua to make analogue more clear.

Ok, got your point

Totktonada added the code health Improve code readability, simplify maintenance and so on label Jun 6, 2018

Totktonada self-assigned this Jun 6, 2018

Totktonada added 6 commits June 8, 2018 19:52

Move our own types to graphql/core/types.lua

1d2d206

Part of #166.

Minor: prettify code of create_default_accessor()

7426d89

Fixed ldoc comment.

Move graphql.utils.show_trace() to test.utils

ede951d

Rename test.utils to test.test_utils

eb9817a

It is to make module name consistent with the variable name by which it is used.

Add GraphiQL license

fc32742

Split code to modules

fb4b75e

Moves are mostly done w/o changes. There are APIs (parameters and return values) that are subject to further refactoring. Removed duplicate code in resolve the function for multihead connections. Part of #166.

Totktonada force-pushed the gh-166-refactor-arguments branch from 0dc051c to 79d227a Compare June 13, 2018 02:10

This was referenced Jun 13, 2018

Test record within a record #46

Closed

Add PCRE matching support for string fields #73

Closed

Totktonada force-pushed the gh-166-refactor-arguments branch from 79d227a to cc7a97f Compare June 13, 2018 02:15

Totktonada force-pushed the gh-166-refactor-arguments branch from cc7a97f to 6d3b455 Compare June 13, 2018 02:18

Support union, array, map in arguments

658438b

Fixes #166.

Totktonada changed the title ~~WIP: Split-up modules and refactor arguments converting~~ Split-up modules and refactor arguments converting Jun 13, 2018

Totktonada requested a review from SudoBobo June 13, 2018 16:33

Totktonada mentioned this pull request Jun 14, 2018

Add notes on new usability features to README.md #110

Closed

Add insert / update argument tests with variables

a3ba71e

Part of #166.

SudoBobo suggested changes Jun 14, 2018

View reviewed changes

SudoBobo reviewed Jun 14, 2018

View reviewed changes

SudoBobo approved these changes Jun 14, 2018

View reviewed changes

Totktonada merged commit 7569a7d into master Jun 14, 2018

Totktonada deleted the gh-166-refactor-arguments branch June 14, 2018 11:39

This was referenced Jun 17, 2018

Umbrella issue for mutation support #142

Closed

Fix introspection and GraphiQL #179

Closed

Totktonada mentioned this pull request Jun 27, 2018

Remove code duplication in multi-head connections #123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split-up modules and refactor arguments converting #168

Split-up modules and refactor arguments converting #168

Totktonada commented Jun 6, 2018

SudoBobo Jun 14, 2018

Totktonada Jun 14, 2018

SudoBobo Jun 14, 2018

Totktonada Jun 14, 2018

SudoBobo Jun 14, 2018

Totktonada Jun 14, 2018

SudoBobo Jun 14, 2018

Totktonada Jun 14, 2018 •

edited

Loading

SudoBobo Jun 14, 2018

SudoBobo Jun 14, 2018

SudoBobo Jun 14, 2018

Totktonada Jun 14, 2018

SudoBobo Jun 14, 2018

Totktonada Jun 14, 2018

SudoBobo Jun 14, 2018

		@@ -0,0 +1,297 @@
		--- Implementation of module-level functions and functions of instances of the
		--- graphql library and a compiled query.

Split-up modules and refactor arguments converting #168

Split-up modules and refactor arguments converting #168

Conversation

Totktonada commented Jun 6, 2018

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

Totktonada Jun 14, 2018 • edited Loading

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

Totktonada Jun 14, 2018 •

edited

Loading