[tarantool-patches] Re: [PATCH] json: added options to json.encode()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for the patch! My congratulations on your
second patch!

See and fix 10 comments below to make your patch better.


1. Please, use imperative in commit header.

Not 'json: added options to json.encode()', but
     'json: add options to json.encode()'.


On 08/07/2018 03:57, Roman Khabibov wrote:
> Added an ability to pass encoder options to json.encode().
> 
> Closes: #2888.
> ---
>   test/app-tap/json.test.lua        | 27 +++++++++++-
>   third_party/lua-cjson/lua_cjson.c | 90 +++++++++++++++++++++++++++++++++------
>   2 files changed, 104 insertions(+), 13 deletions(-)
> 
> diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua
> index 3884b41..1252ad0 100755
> --- a/test/app-tap/json.test.lua
> +++ b/test/app-tap/json.test.lua

2. I will review tests later when we will finish with the other code.

> diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
> index aa8217d..bdfda1e 100644
> --- a/third_party/lua-cjson/lua_cjson.c
> +++ b/third_party/lua-cjson/lua_cjson.c
> @@ -417,23 +417,89 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg,
>       }
>   }
>   
> -static int json_encode(lua_State *l)
> -{
> -    struct luaL_serializer *cfg = luaL_checkserializer(l);
> -    char *json;
> -    int len;
> +#define OPTION(type, name, defvalue) { #name, \
> +	offsetof(struct luaL_serializer, name), type, defvalue}

3. All this code including parse_options is a full copy of
the code from src/lua/util.c. Please, find a way how to do not
duplicate the code. You should consolidate an options parser
function from util.c code and make a declaration in a header.
And then use this function both in util.c in luaL_serializer_cfg()
and in json_encode().

> +static int json_encode(lua_State *l) {
> +	luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1), 1, "expected 1 or 2 arguments");

4. Out of 80 symbols.

>   
> -    json_append_data(l, cfg, 0, &encode_buf);
> -    json = strbuf_string(&encode_buf, &len);
> +	char *json;
> +	int len;

5. In the original code spaces were used, not tabs. Please,
use spaces in this file too.

6. You do not need to annotate those arguments here. Please,
declare them together with assigning.

Not
var name;
name = value;

But
var name = value.

>   
> -    lua_pushlstring(l, json, len);
> +	/* Reuse existing buffer */
> +	strbuf_reset(&encode_buf);
> +	struct luaL_serializer *cfg = luaL_checkserializer(l);
>   
> -    return 1;
> +	/* If have second arg */
> +	if (lua_gettop(l) == 2) {
> +		struct luaL_serializer user_cfg = *cfg;
> +
> +		parse_options(l, &user_cfg);
> +
> +		json_append_data(l, &user_cfg, 0, &encode_buf);
> +	}
> +	/* If have not second arg */
> +	else {

7. Please, put 'else' on the same line as }.

8. Put the dot at the end of sentence.

> +

9. Extra empty line.

> +		json_append_data(l, cfg, 0, &encode_buf);
> +	}
> +
> +	json = strbuf_string(&encode_buf, &len);
> +	lua_pushlstring(l, json, len);
> +
> +	return 1;
>   }

10. Please, add the options to json.decode. API must be symmetrical.

>   
>   /* ===== DECODING ===== */
>