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

Upgrading clap version to 4.x for skysh #284

Merged
merged 1 commit into from Oct 26, 2022

Conversation

sanjayts
Copy link
Contributor

@sanjayts sanjayts commented Oct 23, 2022

New output:

skysh 0.8.0
Sayan Nandan <ohsayan@outlook.com>
The Skytable Shell (skysh)

Usage: skysh [OPTIONS]

Options:
  -C, --sslcert <CERT>          Sets the PEM certificate to use for SSL connections
  -e, --eval [<EXPRESSION>...]  Run one or more expressions without REPL
  -h, --host <HOST>             Sets the remote host to connect to [default: 127.0.0.1]
  -p, --port <PORT>             Sets the remote port to connect to [default: 2003]
      --help                    Print help information
  -V, --version                 Print version information

old output:

Skytable Shell 0.8.0
Sayan N. <ohsayan@outlook.com>
The Skytable Shell (skysh)

USAGE:
    skysh [OPTIONS]

FLAGS:
        --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -C, --sslcert <cert>          Sets the PEM certificate to use for SSL connections
    -e, --eval <expression>...    Run an expression without REPL
    -h, --host <host>             Sets the remote host to connect to
    -p, --port <port>             Sets the remote port to connect to

✔️ By submitting this pull request, I agree to the CLA at: https://cla.skytable.io/skytable/skytable

@glydr
Copy link
Collaborator

glydr commented Oct 23, 2022

CLA assistant check
All committers have signed the CLA.

@ohsayan
Copy link
Member

ohsayan commented Oct 23, 2022

Thank you so much! Just confirming, this upgrades clap for skysh, right? In that case, could you please unlink the issue so that it's not closed when we merge this? Thanks! If you are going to add more commits, please ignore :D

@sanjayts
Copy link
Contributor Author

sanjayts commented Oct 23, 2022

You are welcome! I've removed the issue link.

And correct, this upgrades clap only for skysh. What are your thoughts on handling the changes across other compilation units? All in one PR or let us go at it one at a time? If the latter, I'll unlink the issue and then start working on skybench or something in a separate branch. WDYT?

@ohsayan
Copy link
Member

ohsayan commented Oct 23, 2022

You can do one at a time or more, whatever works/is convenient for you. skyd will require some special treatment though so I think we can leave that for another PR.

Copy link
Member

@ohsayan ohsayan left a comment

Choose a reason for hiding this comment

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

You can also consider removing the cli/src/cli.yml file since after the upgrade that will no longer be required

@ohsayan ohsayan changed the title Upgrading clap version to 4.x Upgrading clap version to 4.x for skysh Oct 23, 2022
Copy link
Member

@ohsayan ohsayan left a comment

Choose a reason for hiding this comment

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

This is looking perfect. Let's leave this PR for skysh and do the others in separate PRs

@ohsayan ohsayan added C-enhancement New feature or request D-client Related to the CLI client hacktoberfest-accepted PR accepted for Hacktoberfest labels Oct 23, 2022
@glydr glydr requested a review from ohsayan October 24, 2022 07:33
@glydr
Copy link
Collaborator

glydr commented Oct 24, 2022

@ohsayan I have dismissed your earlier review

cli/README.md Outdated Show resolved Hide resolved
@ohsayan
Copy link
Member

ohsayan commented Oct 26, 2022

Merged, thanks!

@ohsayan
Copy link
Member

ohsayan commented Oct 30, 2022

@sanjayts can you please add:

---
✔️  By submitting this pull request, I agree to the CLA at: https://cla.skytable.io/skytable/skytable

to the bottom of your original comment like in other pull requests? We need to keep it for compliance. Thanks!

@sanjayts
Copy link
Contributor Author

Done @ohsayan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request D-client Related to the CLI client hacktoberfest-accepted PR accepted for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants