issue#20 Improve CLI operations #43

THLiu55 · 2024-03-15T18:35:45Z

#20
To parse a new command, only one line needs to be added in CMD_KWARGS. In the case of the 'create' operation:

Original:
"create /path data <bool_1> <bool_2>"
where <bool_1> means 'ephemeral' and <bool_2> means 'sequential'.

The line added to CMD_KWARGS:
: [{possible signal for <bool_1>}, {possible signal for <bool_2>}],
"create": [{"-e", "--ephemeral"}, {"-s", "--sequence"}],

effect:
[fk: 2024-03-15 18:07:25.493820 aws:faaskeeper-dev(CONNECTED) session:fb4f71fb 0] create -e /test_e test
{"path": "/test_e", "version": {"created": {"system": [18884638744342255616]}}}

[fk: 2024-03-15 18:25:24.033406 aws:faaskeeper-dev(CONNECTED) session:483f460b 0] create /test_0 test
{"path": "/test_0", "version": {"created": {"system": [18884639021688303616]}}}

[fk: 2024-03-15 18:25:41.538361 aws:faaskeeper-dev(CONNECTED) session:483f460b 1] create -s -e /test_2 test
{"path": "/test_2", "version": {"created": {"system": [18884639027827695616]}}}

next
The prompt for incorrect input should be updated. I think we can restructure the code from line 80 to 89.

EricPyZhou · 2024-03-19T02:29:38Z

Hello there, thank you for the PR. I am a contributor in FaaSKeeper, will share feedbacks ASAP.

sahil-sagwekar2652 · 2024-03-19T12:00:39Z

Hi there, not related to the issue, but I'd like to suggest that you use the backtick syntax to format code into code-block. It will hugely improve the readability.

THLiu55 · 2024-03-19T12:52:25Z

Sorry about the confusions.

def parse_args(cmd: str, args: List[str]):
    if cmd not in CMD_KWARGS:
        return args
    else:
        parsed_args = []
        parsed_kwargs = [False] * len(CMD_KWARGS[cmd])
        for arg in args:
            is_kwarg = False
            for idx, kwarg in enumerate(CMD_KWARGS[cmd]):
                if arg in kwarg:
                    parsed_kwargs[idx] = True
                    is_kwarg = True
                    break
            if not is_kwarg:
                parsed_args.append(arg)
        parsed_args.extend(parsed_kwargs)
        return parsed_args

CMD_KWARGS = {
    "create": [{"-e", "--ephemeral"}, {"-s", "--sequence"}],
    # add more commands format here for parsing
}

To minimize changes to the existing methods, I have written a command translator that translates commands similar to ZooKeeper's commands into the original commands and passes them to the function process_cmd.

I observed that in current user input, all the boolean parameters are at the end of the parameter list. Therefore, we can maintain a boolean list with a length equal to the number of boolean parameters (also equal to the length of CMD_KWARGS[cmd]), and initialize all values to False. As we iterate through the parameters, for each parameter, we check if it exists in CMD_KWARGS[cmd]. If it does, we set the corresponding position in the boolean list to True.

To add new command, we need to ensure that each symbol set in CMD_KWARGS corresponds to the position of boolean parameters in the function. For example, in the create command, the first symbol set in CMD_KWARGS represents ephemeral, and the second symbol set represents sequence, corresponding to the first boolean parameter in the function which represents whether it's ephemeral and the second boolean parameter representing whether it's sequential.

EricPyZhou · 2024-03-19T18:47:51Z

bin/fkCli.py

+            args += f"{arg} "
+        else:
+            flags += f"{arg} "
+    click.echo(f"Command: {args}| Flags: {flags}")


we can provide users with examples that are syntactically correct. e.g.
print_cmd_info('create') generates:
create -s -e /test 0x0

EricPyZhou · 2024-03-19T18:51:48Z

bin/fkCli.py

+# status code: 0 - success, 1 - not need to parse, 2 - error  
+def parse_args(cmd: str, args: List[str]):
+    if cmd not in CMD:
+        return args, PARSE_SUCCESS


Im a bit confused by the Status code, PARSE_SUCCESS is used here and at the end of a successful mapping.

My mistake, for now "PARSE_SUCCESS" is useless. I'm considering whether to put input check in parser step or later in process_cmd function which will require some modification in it.

In my opinion, parser/mapper would map user inputs to the defined syntax amd let process_cmd to do actual content checking.

My mistake, for now "PARSE_SUCCESS" is useless. I'm considering whether to put input check in parser step or later in process_cmd function which will require some modification in it.

~~and I personally think that, to align with ZK cli, we do not want syntaxes like:~~ ~~create path value [bool_arguments]~~
~~We only want user inputs in the format of:~~
create [bool_arguments] path value
~~Which should be the responsibility of a parser/mapper to check for such things~~

I thought we needed to allow [bool_arguments] to appear in any position...
If "create path value [bool_arguments]" is incorrect syntax, I may need to revise my parser implementation.

I thought we needed to allow [bool_arguments] to appear in any position... If "create path value [bool_arguments]" is incorrect syntax, I may need to revise my parser implementation.

My bad, I test with zookeeper and it does allow [bool_arguments] to be anywhere. You are right, thank you for pointing that out.

EricPyZhou · 2024-03-19T18:55:34Z

bin/fkCli.py

+    # create /test 0x0 False False
+    "create": [ARGS["path"], ARGS["data"], KWARGS["ephemeral"], KWARGS["sequence"]],  
+    # test /test False True 0x0
+    "test": [ARGS["path"], KWARGS["boolean1"], KWARGS["boolean2"], ARGS["data"]],


Since def parse_args(cmd: str, args: List[str]) looks like have a flexibility to support other cmds like get_data and set_data, please add them.

EricPyZhou · 2024-03-19T18:57:34Z

bin/fkCli.py

+# find the position of the argument in the command (function call like), return -1 if the argument is not a flag
+def kwarg_pos(arg: str, kwargs_array: List[str]):
+    for idx, kwarg in enumerate(kwargs_array):
+        if (type(kwarg) is set) and (arg in kwarg):


please use isinstance() to keep consistent

EricPyZhou · 2024-03-20T01:55:22Z

bin/fkCli.py

+                click.echo(f"Command Example: {INPUT_FORMAT[cmd]}")
+                return client.session_status, client.session_id
+            if isinstance(args[idx], bool): # convert boolean to string
+                args[idx] = str(args[idx])


What is the reason for converting to string? is it because of line 148?

and line 140

and line 140

Ya. Great to see the latest commit, does look cleaner. I will test each command ASAP.

EricPyZhou · 2024-03-20T03:29:36Z

Thanks for the work, I test the cmds locally.

Since I dont have commit permission to your forked repo, here is something simple that can be put in the test folder:

import pytest

from bin.fkCli import parse_args

def test_cli():
    print("test create")
    assert ['/test', '0x0', True, True] == parse_args("create", ["-e", "-s", "/test", "0x0"])[1]
    assert ['/test', '0x0', True, True] == parse_args("create", ["/test", "0x0", "-e", "-s"])[1]
    assert ['/test', '0x0', False, False] == parse_args("create", ["/test", "0x0"])[1]
    assert ['/test', '0x0', True, False] == parse_args("create", ["/test", "0x0", "-e"])[1]
    assert ['/test', '0x0', False, True] == parse_args("create", ["/test", "0x0", "-s"])[1]
    print("test get")
    assert ['/test', True] == parse_args("get", ["/test", "-w"])[1]
    assert ['/test', True] == parse_args("get", ["-w", "/test"])[1]
    assert ['/test', False] == parse_args("get", ["/test"])[1]
    print("test getChildren")
    assert ['/test', True] == parse_args("getChildren", ["/test", "-i"])[1]
    assert ['/test', True] == parse_args("getChildren", ["-i", "/test"])[1]
    assert ['/test', False] == parse_args("getChildren", ["/test"])[1]

just dont forget to add two __init__.py files into folders bin and tests

In the future, we can utilize the signature function and faaskeeper client to check for the syntax, but this is not the priority.

Edit: correct the assertion for -e and -s

THLiu55 · 2024-03-20T09:59:26Z

Thank you for your review. I added the test and I'm bit confused about these two test case:
assert ['/test', '0x0', False, False] == parse_args("create", ["/test", "0x0", "-e"])[1]
assert ['/test', '0x0', False, False] == parse_args("create", ["/test", "0x0", "-s"])[1]

EricPyZhou · 2024-03-20T14:08:59Z

Thank you for your review. I added the test and I'm bit confused about these two test case:
assert ['/test', '0x0', False, False] == parse_args("create", ["/test", "0x0", "-e"])[1]
assert ['/test', '0x0', False, False] == parse_args("create", ["/test", "0x0", "-s"])[1]

Oh my bad, I only changed that Bool in my local env and forgot to change accordingly in the comments text. Thank you for pointing that out.

parse user command to function call

35b09d7

function call independent version

38a1f26

EricPyZhou reviewed Mar 19, 2024

View reviewed changes

Tianhao added 2 commits March 19, 2024 23:36

add supporting cmds & user input check

6fdc39b

resolve variable name conflict

ea89757

EricPyZhou reviewed Mar 20, 2024

View reviewed changes

remove the code to convert bool to str

2efc341

CLI parser tests

adc732b

EricPyZhou approved these changes Mar 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue#20 Improve CLI operations #43

issue#20 Improve CLI operations #43

THLiu55 commented Mar 15, 2024

EricPyZhou commented Mar 19, 2024

sahil-sagwekar2652 commented Mar 19, 2024

THLiu55 commented Mar 19, 2024

EricPyZhou Mar 19, 2024

EricPyZhou Mar 19, 2024

THLiu55 Mar 19, 2024

EricPyZhou Mar 19, 2024 •

edited

Loading

EricPyZhou Mar 19, 2024 •

edited

Loading

THLiu55 Mar 19, 2024

EricPyZhou Mar 19, 2024

EricPyZhou Mar 19, 2024

EricPyZhou Mar 19, 2024

EricPyZhou Mar 20, 2024

THLiu55 Mar 20, 2024

EricPyZhou Mar 20, 2024

EricPyZhou commented Mar 20, 2024 •

edited

Loading

THLiu55 commented Mar 20, 2024

EricPyZhou commented Mar 20, 2024

issue#20 Improve CLI operations #43

Are you sure you want to change the base?

issue#20 Improve CLI operations #43

Conversation

THLiu55 commented Mar 15, 2024

EricPyZhou commented Mar 19, 2024

sahil-sagwekar2652 commented Mar 19, 2024

THLiu55 commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricPyZhou Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

EricPyZhou Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricPyZhou commented Mar 20, 2024 • edited Loading

THLiu55 commented Mar 20, 2024

EricPyZhou commented Mar 20, 2024

EricPyZhou Mar 19, 2024 •

edited

Loading

EricPyZhou Mar 19, 2024 •

edited

Loading

EricPyZhou commented Mar 20, 2024 •

edited

Loading