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

fix: Improves CLI operations for ephemeral and sequential params #31

Closed
wants to merge 1 commit into from

Conversation

afzal442
Copy link

@afzal442 afzal442 commented Apr 3, 2023

For #20

e.g.

[fk: 2023-04-03 19:47:24.456485 aws:faaskeeper-dev(CONNECTED) session:2ee45722 2] create /test 0x0 -e
['/test', '0x0', 'True', 'False']

cc @mcopik

@mcopik
Copy link
Collaborator

mcopik commented Apr 4, 2023

@afzal442 Thanks! The implementation seems pretty fixed for the arguments of the create operation, and it will be difficult to maintain and expand. After all, there are many more commands that we would like to support.

I think the alternative approach posted by @shailendrasingh117 will be more modular. Maybe you can both collaborate on a single solution?

@afzal442
Copy link
Author

afzal442 commented Apr 4, 2023

Thanks @mcopik for the review! we can support for create operation by adding condition if cmd == `create` simply now.

Talking about the alternative approach, I had that same thing figured out but I don't know the use cases of get_data and set_data. That idea support for cmd is limited. What if cmd is sth else like connect if command not in COMMANDS: click.echo(f"Invalid command: {command}"). I'm not sure what it does and how it calls unkown methods.

       create_node(**kwargs)
   elif command == "get_data":
       get_data(**kwargs)
   elif command == "set_data":
       set_data(**kwargs)

LMK what you think.

@shailendrasingh117
Copy link

This approach can also make it difficult to handle errors in a consistent way. If an unknown command is passed to the dispatcher, the current implementation simply prints an error message to the console. However, in a more complex application, it might be more appropriate to raise an exception or return an error code to the caller.
@mcopik @afzal442

@afzal442 afzal442 closed this Apr 14, 2023
@afzal442
Copy link
Author

Closed as no progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants