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

feat(cli): use ticker format for addpair #1522

Merged
merged 1 commit into from
May 7, 2020
Merged

Conversation

sangaman
Copy link
Collaborator

@sangaman sangaman commented May 6, 2020

This modifies the addpair command to use the trading pair ticker format such as LTC/BTC used in different commands, while keeping the ability to specify currencies separately.

Related issue #1521.

@sangaman sangaman added the command line (CLI) Relating to the command line interface tools label May 6, 2020
@sangaman sangaman self-assigned this May 6, 2020
@raladev
Copy link
Contributor

raladev commented May 6, 2020

@sangaman

  1. addpair BTC/LTC works
  2. I cant specify currencies separately. I think that one of the following commands should work :) :
    ./xucli -p 28886 addpair BTC LTC
    ./xucli -p 28886 addpair --base_currency BTC --quote_currency LTC
    ./xucli -p 28886 addpair -b=BTC -q=LTC
    ./xucli -p 28886 addpair -b BTC -q LTC
    bad_attempts.log
  3. also I tried ./xucli -p 28886 addpair --pair=BTC/LTC but it does not work too and I dont understand why I cant use --pair option as help saids (I think its connected with previous point).

@kilrau

  1. U wrote that addpair BTC LTC should work too. addpair -b BTC -q LTC is enough or should it be addpair BTC LTC? Two interfaces by default (without flags) for one command looks a bit strange.


export const command = 'addpair <base_currency> <quote_currency>';
export const command = 'addpair <pair>';

export const describe = 'add a trading pair';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const describe = 'add a trading pair';
export const describe = 'add a trading pair such as LTC/BTC';

@kilrau
Copy link
Contributor

kilrau commented May 6, 2020

Let's keep it simple. This is not worth catching 10 different ways of specifiying a pair. Only the following should work:
addpair LTC/BTC
addpair lTc/bTc

But xucli addpair --help should make that clear. Please paste a screenshot of what you find confusing in this description.

I cant use --pair option as help saids (I think its connected with previous point).
Help doesn't say that, it says addpair <pair>, which means addpair ltc/btc, it does not mean addpair --pair ltc/btc (which doesn't make much sense either).

@kilrau
Copy link
Contributor

kilrau commented May 6, 2020

Looking at the help output, you are right @raladev , these should work

addpair --base_currency LTC --quote_currency BTC
addpair -b LTC -q BTC

Please help @sangaman understand what fails by providing output/error message.

@raladev
Copy link
Contributor

raladev commented May 6, 2020

Help doesn't say that, it says addpair <pair>, which means addpair ltc/btc, it does not mean addpair --pair ltc/btc (which doesn't make much sense either).

it says
--pair the trading pair in ticker format such as LTC/BTC [string]
that means that --pair can be used as a flag and addpair --pair ltc/btc should work

Please help @sangaman understand what fails by providing output/error message.

They are in bad_attempts.log file. I think the problem is that param is mandatory and i cant use flags because I don't pass this parameter.
How it Should be: "pair" or --pair "pair" or -b -q (both)

@kilrau
Copy link
Contributor

kilrau commented May 6, 2020

Gotcha. Since it's in the help it should work, but honestly if it takes longer than 5 minutes to figure it out, I am fine with removing all of these:

--pair the trading pair in ticker format such as LTC/BTC [string]
--base_currency    the currency bought and sold for this trading pair [string]
--quote_currency   the currency used to quote a price                 [string]

and just allow the ltc/btc format. @sangaman

This modifies the `addpair` command to use the trading pair ticker
format such as LTC/BTC used in different commands, while keeping the
ability to specify currencies separately.

Related issue #1521.
@sangaman sangaman force-pushed the cli/addpair-ticker branch from 50a77bc to b8b0f9b Compare May 7, 2020 05:43
@sangaman
Copy link
Collaborator Author

sangaman commented May 7, 2020

Digging into this a bit made me realize that the approach we're using across almost every cli command is somewhat misleading in that it always prints the required, positional command arguments as "Options" in the help output. If you were to actually specify the option by passing it in as a flag instead of a positional argument, such as xucli connect --node_uri 038395febbce... instead of xucli connect 038395febbce... you'd get an error.

I can clarify those other commands in a separate PR. For now I changed this so it only uses positional arguments and that both addpair LTC/BTC and addpair LTC BTC work and the help output is clear with example commands and no misleading option descriptions.

$ ./bin/xucli addpair --help
xucli addpair <pair_id|base_currency> [quote_currency]

add a trading pair

Positionals:
  pair_id, base_currency  the pair ticker id or base currency[string] [required]
  quote_currency          the currency used to quote a price            [string]

Options:
  --help             Show help                                         [boolean]
  --version          Show version number                               [boolean]
  --rpcport, -p      RPC service port                                   [number]
  --rpchost, -h      RPC service hostname                               [string]
  --tlscertpath, -c  Path to the TLS certificate of xud                 [string]
  --json, -j         Display output in json format    [boolean] [default: false]
  --xudir, -x        Data directory for xud                             [string]

Examples:
  xucli addpair LTC/BTC  add the LTC/BTC trading pair by ticker id
  xucli addpair LTC BTC  add the LTC/BTC trading pair by currencies

@raladev raladev merged commit 8d39862 into master May 7, 2020
@ghost ghost deleted the cli/addpair-ticker branch May 7, 2020 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command line (CLI) Relating to the command line interface tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants