[tarantool-patches] Re: [PATCH][vshard] Check self passed to object methods
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jun 1 21:11:29 MSK 2018
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.
More information about the Tarantool-patches
mailing list