Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue#20 Improve CLI operations #43

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 69 additions & 1 deletion bin/fkCli.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,32 @@
"close": "stop",
"connect": "start",
}
# argument should be a string
ARGS = {
"path": "/path:str",
"data": "data:str",
}

# flag should be a set of its representations (strings)
KWARGS = {
"ephemeral": {"-e", "--ephemeral"},
"sequence": {"-s", "--sequence"},
"boolean1": {"-b1", "--boolean1"},
"boolean2": {"-b2", "--boolean2"},
}

# program can distinguish kwarg and arg by their type
CMD = {
# 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"]],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# add more commands format here for parsing...
}
# parse status code
PARSE_SUCCESS = 1
PARSE_ERROR = 0

fkCompleter = WordCompleter(keywords, ignore_case=True)

Expand Down Expand Up @@ -137,6 +163,46 @@ def process_cmd(client: FaaSKeeperClient, cmd: str, args: List[str]):
return client.session_status, client.session_id


# 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):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use isinstance() to keep consistent

return idx
return -1

# print the command format and flags to user
def print_cmd_info(cmd: str):
args = ""
flags = ""
for arg in CMD[cmd]:
if type(arg) is str:
args += f"{arg} "
else:
flags += f"{arg} "
click.echo(f"Command: {args}| Flags: {flags}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



# parse the arguments and return the parsed arguments
# 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@EricPyZhou EricPyZhou Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@EricPyZhou EricPyZhou Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
parsed_args = [False] * len(CMD[cmd])
arg_idx = parse_args_idx = 0
while arg_idx < len(args):
idx = kwarg_pos(args[arg_idx], CMD[cmd])
if idx != -1:
parsed_args[idx] = True
else:
while isinstance(CMD[cmd][parse_args_idx], set): # skip the positions for flags
parse_args_idx += 1
parsed_args[parse_args_idx] = args[arg_idx]
parse_args_idx += 1
arg_idx += 1
return parsed_args, PARSE_SUCCESS


@click.command()
@click.argument("config", type=click.File("r"))
@click.option("--port", type=int, default=-1)
Expand Down Expand Up @@ -190,7 +256,9 @@ def cli(config, port: int, verbose: str):
elif cmd not in keywords:
click.echo(f"Unknown command {text}")
else:
status, session_id = process_cmd(client, cmd, cmds[1:])
print(cmd, cmds[1:])
parsed_args = parse_args(cmd, cmds[1:])
status, session_id = process_cmd(client, cmd, parsed_args)
counter += 1

print("Closing...")
Expand Down