[config] Help text cleanup #10748

Open
wants to merge 9 commits into
from

Conversation

Projects
None yet
5 participants
Member

jnewbery commented Jul 5, 2017

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()

Contributor

practicalswift commented Jul 6, 2017

Concept ACK

@@ -19,6 +19,9 @@
#include "httpserver.h"
#include "httprpc.h"
#include "utilstrencodings.h"
+#ifdef ENABLE_WALLET
@laanwj

laanwj Jul 10, 2017

Owner

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

src/util.h
@@ -44,6 +44,9 @@ class CTranslationInterface
boost::signals2::signal<std::string (const char* psz)> Translate;
};
+extern const int HELP_MESSAGE_FILTER_NONE;
@laanwj

laanwj Jul 10, 2017

Owner

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

jnewbery Jul 10, 2017

Member

I've moved the definition to the header file

@@ -20,6 +20,9 @@
#include "clientversion.h"
#include "init.h"
#include "util.h"
+#ifdef ENABLE_WALLET
@laanwj

laanwj Jul 10, 2017 edited

Owner

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

jnewbery Jul 10, 2017

Member

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

laanwj Jul 10, 2017

Owner

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

jonasschnelli Jul 13, 2017

Member

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

jnewbery Jul 13, 2017

Member

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.

Member

jnewbery commented Jul 10, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment