[config] Help text cleanup #10748

jnewbery commented at 1:27 PM on July 5, 2017: member

This is very much a matter of personal taste, but I think this changeset makes command line arguments help text code clearer.

A bunch of changes:

order groupings alphabetically
order arguments alphabetically within groupings
make HelpMessageOpt() responsible for whether to print debug arguments. This is done by passing a HELP_MESSAGE_FILTER, which will result in the help text being printed only if -debug-help is set
move server-specific option into bitcoind.cpp, and remove the mode argument from HelpMessage()
move the calls to GetWalletHelpString() into bitcoind.cpp and qt (removes one dependency on bitcoin_wallet from bitcoin_server).

the help message filter is implemented as a bitfield, so could be updated to include other filters (eg HELP_MESSAGE_FILTER_NOT_WIN32) and remove the preprocessor conditionals from HelpMessage()

fanquake added the label Docs and Output on Jul 5, 2017

practicalswift commented at 7:31 AM on July 6, 2017: contributor

Concept ACK

in src/bitcoind.cpp:22 in 2bf9c8bc79 outdated

  18 | @@ -19,6 +19,9 @@
  19 |  #include "httpserver.h"
  20 |  #include "httprpc.h"
  21 |  #include "utilstrencodings.h"
  22 | +#ifdef ENABLE_WALLET

laanwj commented at 2:45 PM on July 10, 2017:

Right, we need to move the wallet dependency up here if we want to remove the wallet dependency from libbitcoin_server.

in src/util.h:47 in 2bf9c8bc79 outdated

  43 | @@ -44,6 +44,9 @@ class CTranslationInterface
  44 |      boost::signals2::signal<std::string (const char* psz)> Translate;
  45 |  };
  46 |  
  47 | +extern const int HELP_MESSAGE_FILTER_NONE;

laanwj commented at 2:48 PM on July 10, 2017:

Why not define the flags in the header? That's the usual approach used for constants throughout the code, and has the advantage of making the values available to the compiler so the linker doesn't have to reserve memory for them.

jnewbery commented at 4:16 PM on July 10, 2017:

I've moved the definition to the header file

[config] add help message filters 119163f4b0

[config] use help message filters in init.cpp 144fff685a

[config] [wallet] use help message filters for wallet dd07c73018

[config] remove showDebug var from HelpMessage() 479f2b323f

[config] [bitcoin-cli] fix config help text ordering fed00570ea

[config] tidy up help text ordering in init and wallet a91306bcb2

[config] [qt] tidy up help message ordering in qt 5aeac98eb8

[config] Remove HelpMessage mode parameter ac05e375ef

[config] move calls to GetWalletHelpString() into bitcoind and bitcoin-qt 01e5976fe7

in src/qt/utilitydialog.cpp:23 in 2bf9c8bc79 outdated

  19 | @@ -20,6 +20,9 @@
  20 |  #include "clientversion.h"
  21 |  #include "init.h"
  22 |  #include "util.h"
  23 | +#ifdef ENABLE_WALLET

laanwj commented at 2:50 PM on July 10, 2017:

Please don't introduce a wallet dependency here. Utilitydialog is supposed to be neutral in that regard.

Edit: though I don't really see an alternative either...

jnewbery commented at 3:34 PM on July 10, 2017:

I'm not very familiar with the qt code. I see that there are already plenty of #ifdef ENABLE_WALLET in the qt code already. Is there something special about utilitydialog.cpp ?

I'm very happy to take an alternative approach if you have suggestions.

laanwj commented at 6:43 PM on July 10, 2017:

Is there something special about utilitydialog.cpp ?

No, but adding ENABLE_WALLET should be avoided everywhere if possible. I think it may be unavoidable here, though.

jonasschnelli commented at 7:31 AM on July 13, 2017:

I think a possible solution would be to have a signal for collecting help strings. The signal emitter then could take care of sorting the groups. But probably out of scope.

jnewbery commented at 11:59 AM on July 13, 2017:

possible solution would be to have a signal for collecting help strings

Certainly possible to have a signal for wallet to pass its help strings to server, but qt also has its own help strings (bitcoind should also have its own help string for -server, but that's actually implemented in init.cpp for now). Difficult to see where/how those would all be sorted.

Really I don't think it matters too much, and I'm happy with other approaches, or to drop this PR entirely if people don't think this is an improvement.

jnewbery force-pushed on Jul 10, 2017

jnewbery commented at 4:18 PM on July 10, 2017: member

Addressed @laanwj's comment, squashed fixup commit and rebased.

jnewbery commented at 3:33 PM on August 14, 2017: member

This is a heavily-conflicting PR that will require frequent rebase. I'm not keen on doing that unless people think this is an improvement and should get merged. @laanwj @jonasschnelli you've both taken a look at this and left comments. Can I assume concept ACKs from you? Do you think this is an improvement?

in src/bitcoin-cli.cpp:43 in 01e5976fe7

  36 | @@ -37,15 +37,15 @@ std::string HelpMessageCli()
  37 |      strUsage += HelpMessageOpt("-?", _("This help message"));
  38 |      strUsage += HelpMessageOpt("-conf=<file>", strprintf(_("Specify configuration file (default: %s)"), BITCOIN_CONF_FILENAME));
  39 |      strUsage += HelpMessageOpt("-datadir=<dir>", _("Specify data directory"));
  40 | -    AppendParamsHelpMessages(strUsage);
  41 |      strUsage += HelpMessageOpt("-named", strprintf(_("Pass named instead of positional arguments (default: %s)"), DEFAULT_NAMED));
  42 | +    strUsage += HelpMessageOpt("-rpcclienttimeout=<n>", strprintf(_("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)"), DEFAULT_HTTP_CLIENT_TIMEOUT));
  43 |      strUsage += HelpMessageOpt("-rpcconnect=<ip>", strprintf(_("Send commands to node running on <ip> (default: %s)"), DEFAULT_RPCCONNECT));
  44 | +    strUsage += HelpMessageOpt("-rpcpassword=<pw>", _("Password for JSON-RPC connections"));

MarcoFalke commented at 3:41 PM on August 14, 2017:

Imo rpcpassword belongs to rpcuser, no need to sort alphabetically.

MarcoFalke commented at 3:59 PM on September 7, 2017: member

There seems to be no conceptual acknowledgment to do this. Closing for now.

MarcoFalke closed this on Sep 7, 2017

jnewbery commented at 4:30 PM on September 7, 2017: member

Please re-open. There hasn't been a rejection of this idea.

MarcoFalke commented at 4:51 PM on September 7, 2017: member

Ok, leaving this open for another month. Though, if no one wants to take a look I don't see a reason to keep it open forever.

MarcoFalke reopened this on Sep 7, 2017

jnewbery commented at 5:48 PM on September 14, 2017: member

I think you're right - there's no appetite for this PR. I may break it up and PR some parts of it, since I think they're useful changes.

jnewbery closed this on Sep 14, 2017

DrahtBot locked this on Sep 8, 2021