Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org,
	Roman Khabibov <roman.habibov1@yandex.ru>
Subject: [tarantool-patches] Re: [PATCH] json: added options to json.encode()
Date: Mon, 9 Jul 2018 13:33:33 +0300	[thread overview]
Message-ID: <f6bd7333-7701-cd2e-a527-daab39a909eb@tarantool.org> (raw)
In-Reply-To: <693ca18f728ee462e28861841004ff77bf62bfb5.1531010828.git.roman.habibov1@yandex.ru>

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 ===== */
> 

  reply	other threads:[~2018-07-09 10:33 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <cover.1531010828.git.roman.habibov1@yandex.ru>
2018-07-08  0:57 ` [tarantool-patches] " Roman Khabibov
2018-07-09 10:33   ` Vladislav Shpilevoy [this message]
2018-07-17 18:19     ` [tarantool-patches] " roman.habibov1
2018-07-19 10:18       ` Vladislav Shpilevoy
2018-07-23 22:38         ` [tarantool-patches] Re: [PATCH v3] json: add " roman.habibov1
2018-07-25 21:35           ` Vladislav Shpilevoy
2018-07-26  9:40             ` roman.habibov1
2018-07-26 10:07               ` Vladislav Shpilevoy
2018-07-26 12:29                 ` roman.habibov1
2018-07-26 12:33                   ` Vladislav Shpilevoy
2018-07-26 13:19                     ` roman.habibov1
2018-07-26 21:45                       ` Vladislav Shpilevoy
2018-07-31 15:29                         ` roman.habibov1
2018-08-01 10:37                           ` Vladislav Shpilevoy
2018-08-01 20:41                             ` roman.habibov1
2018-08-02 12:59                               ` Vladislav Shpilevoy
2018-08-07 21:52                                 ` roman.habibov1
2018-08-07 21:53                                   ` roman.habibov1
2018-08-08 19:07                                   ` Vladislav Shpilevoy
2018-08-13 23:14                                     ` roman.habibov1
2018-08-14 22:29                                       ` Vladislav Shpilevoy
2018-08-23 21:03                                         ` Alexander Turenko
2018-09-09 15:28                                         ` Alexander Turenko
2018-09-09 23:42                                           ` roman.habibov1
2018-09-10 13:12                                             ` Alexander Turenko
2018-08-08 19:08                                 ` Vladislav Shpilevoy
2018-07-11  7:57   ` [tarantool-patches] Re: [PATCH] json: added " Kirill Yukhin
2018-07-19 10:24   ` Vladislav Shpilevoy
2018-09-13 15:23   ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f6bd7333-7701-cd2e-a527-daab39a909eb@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=roman.habibov1@yandex.ru \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH] json: added options to json.encode()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox