From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 689A229199 for ; Fri, 1 Jun 2018 13:24:37 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id T6NGsLl8dJeC for ; Fri, 1 Jun 2018 13:24:37 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id AF86629197 for ; Fri, 1 Jun 2018 13:24:36 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH][vshard] Check self passed to object methods References: <20180601124912.9145-1-avkhatskevich@tarantool.org> From: Alex Khatskevich Message-ID: <89742575-b0ce-899f-9881-d64deac8c57c@tarantool.org> Date: Fri, 1 Jun 2018 20:24:34 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy , tarantool-patches@freelists.org Hi. > 1. Please, put links after --- below the commit message. Not before and > not above your own ---. > ok > > 2. Why can not you just iterate over replicaset metatable? Your way > requires to update this test each time a new method is added. I thought, that it can create a lot of pain in case someone adds method which do not take self attribute. Anyway, fixed adding this helper: ----------------------- 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; ------------------------- >> +        connect_replica = function(rs, r) >> +            assert(type(r) == "table", >> +                   'Use replicaset:connect_replica(...) instead of ' .. >> +                   'replicaset.connect_replica(...)')> +            >> return replicaset_connect_to_replica(rs, r) > > 4. You have no checked rs. This check_self_arg is broken - you must > have two functions: > check and generate checker. Now you have only generate checker that > you have named > check_self_arg. > > In this function you do manual check_self_arg for 'r' and 'rs'. For other > functions you do the same as now - generate checker. Fixed this way:             assert(type(rs) == "table" and type(r) == "table", > > 5. Describing a function you may omit 'this function does', 'this > function > is needed to' etc. Function comment must look like a commit title: > imperative text describing what this function shall do. > 7. 'This function takes' must be moved to the corresponding @param. Rewritten a little. > 6. anguments? > 8. modele? > 9. You do not need extra new line after each @param. Fixed Here is a new version -- -- 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. -- -- @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. -- > 10. Why no just 'return function wrapped_func(... ' ? Fixed