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

Redesign CLI help output #1961

Merged
merged 3 commits into from
Nov 24, 2018
Merged

Conversation

cbeams
Copy link
Contributor

@cbeams cbeams commented Nov 24, 2018

Prior to this pull request, Bisq's --help output looked like this:

$ bisq-desktop --help
Option                                  Description
------                                  -----------
--appDataDir <String>                   Application data directory (default:
                                          /Users/cbeams/Library/Application
                                          Support/Bisq)
--appName <String>                      Application name (default: Bisq)
--banList <String>                      Nodes to exclude from network
                                          connections. (default: )
--baseCurrencyNetwork <String>          Base currency network (default:
                                          BTC_MAINNET)
--bitcoinRegtestHost <RegTestHost>      (default: LOCALHOST)
--btcNodes <String>                     Custom nodes used for BitcoinJ as
                                          comma separated IP addresses.
                                          (default: )
[...]

Now it reads as follows:

$ bisq-desktop --help
Bisq Desktop version 0.8.0

Usage: bisq-desktop [options]

Options:

  --help
        This help text

  --logLevel=<OFF|ALL|ERROR|WARN|INFO|DEBUG|TRACE> (default: INFO)
        Log level

  --seedNodes=<host:port[,...]>
        Override hard coded seed nodes as comma separated list e.g.
        'rxdkppp3vicnbgqt.onion:8002,mfla72c4igh5ta2t.onion:8002'

  --myAddress=<host:port>
        My own onion address (used for bootstrap nodes to exclude itself)

  --banList=<host:port[,...]>
        Nodes to exclude from network connections.

  --nodePort=<Integer> (default: 9999)
        Port to listen on

  --useLocalhostForP2P=<Boolean> (default: false)
        Use localhost P2P network for development

  [...]

This new help format is modeled after bitcoind's own help output, which reads as follows:

$ bitcoind --help | head -20
Bitcoin Core Daemon version v0.17.0.0-ge1ed37edaedc85b8c3468bd9a726046344036243

Usage:  bitcoind [options]                     Start Bitcoin Core Daemon

Options:

  -?
       Print this help message and exit

  -alertnotify=<cmd>
       Execute command when a relevant alert is received or we see a really
       long fork (%s in cmd is replaced by message)

  -assumevalid=<hex>
       If this block is in the chain assume that it and its ancestors are valid
       and potentially skip their script verification (0 to verify all,
       default:
       0000000000000000002e63058c023a9a1de233554f28c7b21380b6c9003f36a8,
       testnet:
       0000000000000037a8cd3e06cd5edbfe9dd1dbcc5dacab279376ef7cfc2b4c75)

[...]

The goal is to maximize readibility, make best use of the available 80-character width and to present Bisq's command line interface in a way that is friendly and familiar to those well-versed in *nix idioms.

Reviewers, please look through the commits one-by-one and read the detailed commit comments, thanks.

Prior to this commit, help output for Bisq executables, e.g. Bisq
Desktop itself used JOptSimple's default HelpFormatter implementation,
which creates a quite cramped and hard-to-read output.

This commit introduces a custom HelpFormatter implementation modeled
after bitcoind's own help output. It maximizes readability while making
full use of an 80-character width.
This change eliminates the BisqExecutable.description method and
replaces it with proper use of the `describedAs` and `defaultsTo`
methods in the JOptSimple API. This removes the concern of formatting
option argument descriptions and default values from the BisqExecutable
class, and delegates it to the new BisqHelpFormatter (see previous
commit), which is designed for the purpose.

For example, prior to this commit, the help text for the --banList
option read as follows:

    --banList=<value>
            Nodes to exclude from network connections. (default: )

Now it reads as follows:

    --banList=<host:port[,...]>
            Nodes to exclude from network connections.

Likewise, previous to this commit, the --logLevel option read as
follows:

    --logLevel=<value>
            Log level [OFF, ALL, ERROR, WARN, INFO, DEBUG, TRACE]
            (default: INFO)

And now it reads like this:

    --logLevel=<OFF|ALL|ERROR|WARN|INFO|DEBUG|TRACE> (default: INFO)
            Log level

There are a number of further improvements that can and should be made
to the description text of the various options, the types specified for
their arguments, etc, but these will be handled in subsequent commits.
This commit is strictly about refactoring existing parser configuration
to take advantage of the new BisqHelpFormatter.
Copy link
Contributor

@ManfredKarrer ManfredKarrer left a comment

Choose a reason for hiding this comment

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

utACK

@ManfredKarrer ManfredKarrer merged commit 086fe32 into bisq-network:master Nov 24, 2018
cbeams added a commit to cbeams/bisq that referenced this pull request Dec 5, 2018
This change fixes bisq-network#2048 by removing the assignment of a default value
for the `baseCurrencyNetwork` option at the level of the command line
option parser. The assignment of this default was an oversight in bisq-network#1961
(specifically commit 83e1dd3) that did not account for the fact that
users can change the `baseCurrencyNetwork` value via the Settings screen
in the application. When users change the setting in the application, the new
value is persisted to <appDataDir>/bisq.properties, which is handled at
runtime as a PropertySource with lower precedence than the command line
property source, which means that the changed value is never picked up
because the higher-precedence command line PropertySource always has a
default value.

This fix is surgical in that it addresses only this specific option. A
subsequent change should address the more general issue that setting
defaults in the command line option parser always precludes the
possibility of overriding them in bisq.properties. Basically, we should
revert to the previous strategy of reporting what the default value will
be in the help text without actually assigning a default value in the
option parser using the `defaultsTo` method.
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.

2 participants