Re: [Tarantool-patches] [PATCH] rocks: forward options to luarocks

[Vladislav Shpilevoy <v.shpilevoy@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 <command name> <options> <args>

    <Description>

You did this:

    tarantoolctl <command name> <description>

    <Proposal to use help != description>

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)