From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9A3744696C6 for ; Sun, 5 Apr 2020 21:40:06 +0300 (MSK) References: <1090ea3aa089beee61c44d0e42b3bcf6ea250782.1585171743.git.lvasiliev@tarantool.org> <1d9a9c7f-81b8-5c52-7d7e-907bcedbdf3b@tarantool.org> From: Vladislav Shpilevoy Message-ID: <5280ed92-4ee4-7ce2-3c44-732391124a80@tarantool.org> Date: Sun, 5 Apr 2020 20:40:05 +0200 MIME-Version: 1.0 In-Reply-To: <1d9a9c7f-81b8-5c52-7d7e-907bcedbdf3b@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH] rocks: forward options to luarocks List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: lvasiliev Cc: tarantool-patches@dev.tarantool.org >>> +    shift_argv(lua_arguments, 0, 1) >>>       -- Call LuaRocks >>> -    command_line.run_command('', rocks_commands, nil, unpack(positional_arguments)) >>> +    command_line.run_command('LuaRocks main command-line interface', >> >> 3. I thought we should not print any 'LuaRocks' text when run >> 'tarantoolctl rocks'. So why did you change this? > Sorry, but I don't understand you. > Now we use luarocks help for luarocks. > Do you propose changing the Usage section? No. I am saying that you changed 'LuaRocks' to 'tarantoolctl' in luarocks/lvasiliev/gh-4629-add-chdir-to-make in some seemingly random places. And here you again use 'LuaRocks'. But now I understood why. You use 'tarantoolctl' as a command name, but 'LuaRocks' as the module name. Which can be accessed via 'tarantoolctl' command among other ways. Also I just noticed a bad indentation here. Second line of command_line.run_command should be aligned by (. >>> +        rocks_commands, nil, unpack(lua_arguments)) >>>   end >>>     local function exit_wrapper(func) >>> @@ -1186,117 +1186,14 @@ local commands = setmetatable({ >>>       }, rocks = { >>>           func = exit_wrapper(rocks), process = process_remote, help = { >>>               header = >>> -                "%s rocks [install|make|remove|show|search|list|pack|unpack|doc|test|make_manifest|help]", >>> +                "%s rocks - package managment", >> >> 4. 'managment' -> 'management'. Also none of the commands print >> it like that. They print description and command line separately. >> See 'tarantoolctl help'. In the command line after 'rocks' should >> be said something like 'COMMAND'. And in the description you can >> provide, well, description. > managment has beeb fixed. > LuaRocks(rocks) is a submodule. I don't want to copy-paste all helps for all command with all flags (This is one of the goals of the task). Before the help was huge and useless. All other commands don't use submodules. The help wasn't huge. It was small and consisted of one line listing possible commands. I didn't say what you cited above. I said that you violated the 'help' text format. It is supposed to be in the form: tarantoolctl You did this: tarantoolctl So if someone taps 'tarantoolctl --help', he won't even understand what is 'tarantoolctl rocks' and why would he need to see its 'help' section. Also I didn't say I want you to copy-paste all commands from luarocks and their helps. I said I want a description. Since you seem not to be willing to do that, let me write it for you below. Please, apply that diff unless you have something against this. In the latter case lets discuss. ==================== diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in index b34f65558..f7dac4c9c 100755 --- a/extra/dist/tarantoolctl.in +++ b/extra/dist/tarantoolctl.in @@ -1186,10 +1186,14 @@ local commands = setmetatable({ }, rocks = { func = exit_wrapper(rocks), process = process_remote, help = { header = - "%s rocks - package management", + "%s rocks [OPTIONS] COMMAND", description = [=[ - See tarantoolctl rocks help + Tarantool package manager. To pack/unpack/install/remove + LuaRocks packages, and many more other actions. For more + information see + + tarantoolctl rocks --help ]=], weight = 100, deprecated = false, ==================== >>>               description = >>>   [=[ >>> -        Package management. >>> +        See tarantoolctl rocks help >>>   ]=], >>> @@ -1411,7 +1305,10 @@ local function populate_arguments() >>>   end >>>     local function main() >>> -    populate_arguments() >>> +    if command_name ~= 'rocks' then -- rocks parse arguments themselves Please, don't put comments on the same line as code, start them with a capital letter, and end with a dot. >>> +        populate_arguments() >>> +    end >> >> 5. You probably don't need to change that. Let it parse. >> You don't use the result anyway. > We don't want to have double parsing. And this doesn't make any sense. What is it? A tool called 1000000 times per second? This is not perf critical code at all. But it is highly complicated due to how many tasks tarantoolctl should do. And I want to keep it simple and moduled. Your code currently violates encapsulation, because argument parser seems to be an isolated piece of code, which does not make exceptions for particular commands. Its task is just parse. And you added some command-specific logic in here. Also I tried to remove this check, and it appeared to be totally not related to 'double parsing problem'. The real problem is that taranctoolctl in the argument list looks for some names and may do something unexpected depending on what it found. After reverting this change I called 'taranctoolctl rocks --help' and got just a help section from 'taranctoolctl', not the whole 'help from 'luarocks': $ ./extra/dist/tarantoolctl rocks --help Usage: tarantoolctl rocks - package management See tarantoolctl rocks help But it does not mean you should break encapsulation here. You can add a flag like 'is_self_sufficient' to elements of 'commands' array, and set it to true for 'rocks'. Then you do this: diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in index b34f65558..37eb89a39 100755 --- a/extra/dist/tarantoolctl.in +++ b/extra/dist/tarantoolctl.in @@ -1305,11 +1305,11 @@ local function populate_arguments() end local function main() - if command_name ~= 'rocks' then -- rocks parse arguments themselves + local cmd_pair = commands[command_name] + if not cmd_pair.is_self_sufficient then populate_arguments() end - local cmd_pair = commands[command_name] if #arg < 2 then log.error("Not enough arguments for '%s' command\n", command_name) usage(command_name)