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

Review CLI interface & implement --version #232

Merged
merged 9 commits into from
May 7, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented May 7, 2019

Issue Number

#229

Overview

  • I have grouped command by resources (instead of actions)
  • I have implemented --version using the version from the cardano-wallet.cabal
  • I have added the missing [--port] option to the delete wallet command
  • I have handled more nicely 404 and 409 errors
  • I have handled more nicely what gets printed to stderr and what gets printed to stdout
  • I have made the walletId a positional argument (rather than being a option)
  • I have enabled the PUT /v2/wallets/{walletId} command in the CLI definition
  • I have improved acknowledgement of success with a clear "Ok." message
  • I have removed the mask on the mnemonic prompt (now displayed in clear)
  • I have reviewed the return type of DELETE /v2/wallets/{walletId} to display nothing but a confirmation or error message (was displaying an empty array before)
  • I have fixed the ANSI colors to only be present if the given output handle is a terminal

Comments

@KtorZ KtorZ requested a review from piotr-iohk May 7, 2019 10:31
@KtorZ KtorZ self-assigned this May 7, 2019
cardano-wallet wallet list [--port=INT]
cardano-wallet wallet get [--port=INT] --wallet-id=STRING
cardano-wallet wallet create [--port=INT] --name=STRING [--address-pool-gap=INT]
cardano-wallet wallet delete [--port=INT] --id=STRING
Copy link
Contributor

Choose a reason for hiding this comment

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

--wallet-id=STRING ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes? 😅 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes... or cardano-wallet wallet get [--port=INT] --id=STRING :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! That's what you meant 😄 ... I've just made this a positional argument actually (as discussed in the ticket), so I'll push on top of this commit.

@@ -182,6 +192,18 @@ exec manager args
wId <- args `parseArg` longOption "id"
runClient $ void $ deleteWallet (ApiT wId)

| args `isPresent` longOption "version" = do
let cabal = B8.unpack $(embedFile "cardano-wallet.cabal")
Copy link
Contributor

Choose a reason for hiding this comment

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

we're not shipping cardano-wallet.cabal, so it wouldn't work, would it? :/

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, the file is embedded at compile-time. This won't even compile without the file.

@KtorZ KtorZ force-pushed the KtorZ/229/review-cli-interfaces branch from e1776f7 to af0a8cb Compare May 7, 2019 12:34
port <- args `parseArg` longOption "port"
let env = mkClientEnv manager (BaseUrl Http "localhost" port "")
res <- runClientM cmd env
case res of
Left (FailureResponse r) | responseStatusCode r == status404 -> do
let typ = T.pack $ tyConName $ typeRepTyCon $ typeRep $ Proxy @b
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a similar one for status409 :: Conflict for already existing resources.

@KtorZ KtorZ force-pushed the KtorZ/229/review-cli-interfaces branch from 3c3b4df to 93e6a65 Compare May 7, 2019 13:42
@KtorZ KtorZ mentioned this pull request May 7, 2019
9 tasks
@KtorZ KtorZ merged commit 564a50d into master May 7, 2019
@KtorZ KtorZ deleted the KtorZ/229/review-cli-interfaces branch May 7, 2019 15:12
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.

None yet

3 participants