spcl / faaskeeper

A fully serverless implementation of the ZooKeeper coordination protocol.
BSD 3-Clause "New" or "Revised" License
16 stars 13 forks source link

issue#20 Improve CLI operations #43

Open THLiu55 opened 4 months ago

THLiu55 commented 4 months ago

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 " where means 'ephemeral' and means 'sequential'.

The line added to CMD_KWARGS:

: [{possible signal for }, {possible signal for }], "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 commented 4 months ago

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

sahil-sagwekar2652 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.