Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org,
	AKhatskevich <avkhatskevich@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH][vshard] Check self passed to object methods
Date: Fri, 1 Jun 2018 21:11:29 +0300	[thread overview]
Message-ID: <2404a222-e13e-6bd8-16ee-4f654ff452ea@tarantool.org> (raw)
In-Reply-To: <a1da9b35-0b34-f405-28e7-fb4eac636206@tarantool.org>

Thanks for the fixes!

Please, after fixes put the complete patch at the end of a letter.

See 6 comments below.

> diff --git a/test/router/router.result b/test/router/router.result
> index 5fcabc6..b00cd12 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -977,6 +977,75 @@ util.check_error(vshard.router.sync, "xxx")
>  vshard.router.sync(100500)
>  ---
>  ...
> +-- Check that user passed self arg

1. Please, beforehand describe the issue:
--
-- gh-####: issue description.
--

> +get_object_method_names = function(object)
> +    local res = {}
> +    for name, _ in pairs(getmetatable(object).__index) do
> +        table.insert(res, name)
> +    end
> +    table.sort(res)
> +    return res
> +end;

2. Why do you need table.sort here?

> +for _, func_name in pairs(get_object_method_names(replicaset)) do
> +    local ok, msg = pcall(replicaset[func_name], "arg_of_wrong_type")
> +    table.insert(error_messages, msg:match("Use .*"))
> +end;

3. Why does not work just iteration over getmetatable(object).__index ?

> diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua
> index d210973..ede53c7 100644
> --- a/vshard/replicaset.lua
> +++ b/vshard/replicaset.lua
> @@ -364,30 +365,39 @@ end
>  --
>  local replicaset_mt = {
>      __index = {
> -        connect = replicaset_connect_master;
> -        connect_master = replicaset_connect_master;
> -        connect_all = replicaset_connect_all;
> -        connect_replica = replicaset_connect_to_replica;
> -        rebind_connections = replicaset_rebind_connections;
> -        down_replica_priority = replicaset_down_replica_priority;
> -        up_replica_priority = replicaset_up_replica_priority;
> -        call = replicaset_master_call;
> -        callrw = replicaset_master_call;
> -        callro = replicaset_nearest_call;
> +        connect = csa("replicaset", "connect", replicaset_connect_master);
> +        connect_master = csa("replicaset", "connect_master",
> +                             replicaset_connect_master);
> +        connect_all = csa("replicaset", "connect_all", replicaset_connect_all);
> +        connect_replica = function(rs, r)
> +            assert(type(rs) == "table" and type(r) == "table",
> +                   'Use replicaset:connect_replica(...) instead of ' ..
> +                   'replicaset.connect_replica(...)')
> +            return replicaset_connect_to_replica(rs, r)
> +        end;

4. Looks like you did not that I asked for. Please, do not use assert, and do not
inline part of check_self_arg. Make two functions: check_self_arg, generate_checker
or something.

> +--
> +-- Wrap a given function to check that self argument is passed.
> +-- New function returns an error in case one called a method
> +-- like object.func instead of object:func.
> +-- Returning wrapped function to a user and using raw functions inside of a
> +-- module improves speed.
> +-- This function can be used only in case the second argument is not of the
> +-- "table" type.
> +--

5. Still extra line.

> +-- @param obj_name Name of the called object. Used only for error message
> +--        construction.
> +-- @param func_name Name of the called function. Used only for an error message
> +--        construction.
> +-- @param func A function which is about to be wrapped
> +--
> +-- @retval Wrapped function.
> +--

6. Please, fit the comment in 66 symbols.

  parent reply	other threads:[~2018-06-01 18:11 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-01 12:49 [tarantool-patches] " AKhatskevich
2018-06-01 14:51 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-01 17:24   ` Alex Khatskevich
2018-06-01 18:11   ` Vladislav Shpilevoy [this message]
2018-06-02 10:01     ` Alex Khatskevich
2018-06-03 16:56       ` Vladislav Shpilevoy

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=2404a222-e13e-6bd8-16ee-4f654ff452ea@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=avkhatskevich@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH][vshard] Check self passed to object methods' \
    /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