Chainparams: Translations: DRY: options and error strings #6235

jtimon commented at 8:02 PM on June 4, 2015: contributor

Make some strings translatable when they weren't. Don't duplicate error or help translation strings related to chainparams.

laanwj added the label Improvement on Jun 5, 2015

jtimon force-pushed on Jun 21, 2015

jtimon commented at 12:49 PM on June 21, 2015: contributor

Needed rebase. Also replaced GetParamsHelpMessages with AppendParamsHelpMessages, which optionally takes a showDebugHelp boolean.

jtimon force-pushed on Jun 21, 2015

jtimon commented at 6:53 PM on June 25, 2015: contributor

@theuni this may interact with your code.

jtimon commented at 9:25 PM on June 25, 2015: contributor

Closing for now to reduce noise

jtimon closed this on Jun 25, 2015

jtimon reopened this on Jul 6, 2015

jtimon force-pushed on Jul 6, 2015

jtimon commented at 9:34 PM on July 6, 2015: contributor

Great. MAX_NETWORK_TYPES is removed in the second commit.

rustyrussell commented at 6:24 AM on July 7, 2015: contributor

Ack (tested).

in src/chainparams.cpp:None in e9c4ee4073 outdated

 243 | @@ -243,31 +244,20 @@ const CChainParams &Params() {
 244 |      return *pCurrentParams;
 245 |  }
 246 |  
 247 | -CChainParams &Params(CBaseChainParams::Network network) {
 248 | -    switch (network) {
 249 | -        case CBaseChainParams::MAIN:
 250 | +CChainParams& Params(const std::string& chain)
 251 | +{
 252 | +    if (chain == CBaseChainParams::MAIN)

Diapolo commented at 6:45 AM on July 7, 2015:

Why move away from a switch case construct?

rustyrussell commented at 6:58 AM on July 7, 2015:

You can't switch on a string...

jgarzik commented at 6:46 AM on July 7, 2015: contributor

General approach OK. Comments:

Still repeating a chain lookup in chainparams.cpp & chainparamsbase.cpp
Only now it is worse because it is a more expensive (now duplicated) string comparison
Seeing this duplication hints that a table-driven approach, with each string inside the param struct, is superior to manually administered case statements (& duplicated) of chain strings.

Seems like you want find_chain_by_string(), returns id, which can be passed around efficiently as 'network' -- which is consistent with what the rest of the code does now.

rustyrussell commented at 7:16 AM on July 7, 2015: contributor

@jgarzik since each param has a strNetworkID which is exactly the same as the CBaseChainParams constants introduced, I think a set would be the simplest.

Re-introducing an intermediate type would defeat the point of these patches for me, which is to allow me to easily create a new testnet. I've been hacking bitcoind to run a megablocks testnet, for example.

But I like the simplicity of the patch's mechanical translation; easy to review. A lookup mechanism on top could work well as a new patch.

jtimon commented at 7:58 AM on July 7, 2015: contributor

Still repeating a chain lookup in chainparams.cpp & chainparamsbase.cpp

Only now it is worse because it is a more expensive (now duplicated) string comparison

Yes, the string comparison is more expensive, but it's done on init or tests, so I think it's fine. What do you mean by "now duplicated". Can you point out what exactly am I duplicating here?

-Seeing this duplication hints that a table-driven approach, with each string inside the param struct, is superior to manually administered case statements (& duplicated) of chain strings. Seems like you want find_chain_by_string(), returns id, which can be passed around efficiently as 'network' -- which is consistent with what the rest of the code does now. @jgarzik I'm not sure I understand your suggestion. You mean a function like CBaseChainParams::Network networkEnumFromChainName(std::string)? That doesn't remove the duplicated switch/elifs for chainparams and basechainparams. The problem here with that duplication is having base chainparams in the first place. This is not doing anything against or in favor to that. I believe we could move the datadir to regular chainparams and have users select the bitcoin-cli port manually instead of indirectly through -testnet and -regtest. Anyway, that would belong in another PR. Anyway, this PR was open without replacing the enum with a string. I could reopen that again, but it's cleaner this way and there's many other things that require that replacement in #6382 .

jtimon commented at 5:00 PM on July 28, 2015: contributor

EDIT (after reverting #6077 ): ~~This (and thus #6382) should fail the compilation if merged after #6077, I will rebase both: but don't merge this now.~~

jtimon commented at 11:26 PM on August 20, 2015: contributor

ping @theuni . This should be useful for #6526 just like it's useful for #6382

jtimon force-pushed on Aug 21, 2015

jtimon commented at 11:35 PM on August 21, 2015: contributor

Rebased and fixed (see #6077 (comment) ).

in src/chainparamsbase.cpp:None in e52fee4d94 outdated

  19 | +    strUsage += HelpMessageGroup(_("Chain selection options:"));
  20 | +    strUsage += HelpMessageOpt("-testnet", _("Use the test chain"));
  21 | +    if (debugHelp) {
  22 | +        strUsage += HelpMessageOpt("-regtest", _("Enter regression test mode, which uses a special chain in which blocks can be solved instantly.") + " " +
  23 | +                                   _("This is intended for regression testing tools and app development.") + " " +
  24 | +                                   _("In this mode -genproclimit controls how many blocks are generated immediately."));

sipa commented at 12:17 AM on August 26, 2015:

This last line is outdated. That behaviour moved to the separate generate RPC call.

sipa commented at 12:32 AM on August 26, 2015: member

Untested ACK

theuni commented at 6:25 PM on August 26, 2015: member

ut ACK

jtimon force-pushed on Aug 26, 2015

jtimon commented at 7:26 PM on August 26, 2015: contributor

Fixed @sipa 's nit.

jgarzik commented at 3:14 PM on September 16, 2015: contributor

ut ACK - though this pushes the bounds of my refactor size-o-meter for end-user utility versus source code disturbance ;p

dcousens commented at 4:08 AM on September 17, 2015: contributor

utACK

jtimon commented at 1:48 PM on September 17, 2015: contributor

For more context on more long-term utility of this change see #6382 (needs rebase on top of #6672 ).

jtimon commented at 12:47 PM on September 22, 2015: contributor

ping

apoelstra commented at 2:44 PM on October 2, 2015: contributor

ut ACK. I'm not concerned about string comparisons on init.

With #6382 NACK'd there is less direct utility to this patch, but I like the deduplication of the error handling. (The whole "selecting chainparams" area of the code can be frustrating to read because it has a lot of redundancy like this.)

in src/chainparamsbase.cpp:None in e32640fe2d outdated

  17 | +void AppendParamsHelpMessages(std::string& strUsage, bool debugHelp)
  18 | +{
  19 | +    strUsage += HelpMessageGroup(_("Chain selection options:"));
  20 | +    strUsage += HelpMessageOpt("-testnet", _("Use the test chain"));
  21 | +    if (debugHelp) {
  22 | +        strUsage += HelpMessageOpt("-regtest", _("Enter regression test mode, which uses a special chain in which blocks can be solved instantly.") + " " +

laanwj commented at 11:32 AM on October 20, 2015:

Options help that is only shown with debugHelp should not be translated (see doc/translation_strings_policy.md)

jtimon commented at 11:56 AM on October 20, 2015:

I didn't know that, thanks. I will correct it.

laanwj commented at 11:33 AM on October 20, 2015: member

ut ACK

in src/chainparamsbase.cpp:None in e32640fe2d outdated

 105 | -    default:
 106 | -        assert(false && "Unimplemented network");
 107 | -        return;
 108 | -    }
 109 | +    else
 110 | +        throw std::runtime_error(strprintf(_("%s: Unknown chain %s."), __func__, chain));

MarcoFalke commented at 11:39 AM on October 20, 2015:

This one as well? https://github.com/bitcoin/bitcoin/blob/master/doc/translation_strings_policy.md#do-not-translate-internal-errors

jtimon commented at 12:00 PM on October 20, 2015:

Well, I believe this is an initialization error rather than an internal error. But the initialization errors translation in init.o::AppInit2() seems inconsistent (some times they are translated, some times they aren't). @laanwj can you help us here?

Chainparams: Replace CBaseChainParams::Network enum with string constants (suggested by Wladimir) f3525e24e3

Chainparams: Translations: DRY: options and error strings

Also remove SelectBaseParamsFromCommandLine and SelectParamsFromCommandLine

55a89751fa

jtimon force-pushed on Oct 20, 2015

jtimon commented at 12:31 PM on October 20, 2015: contributor

Updated without translating errors or messages that are only shown when debugHelp is true (solved @laanwj and @MarcoFalke 's nits).

laanwj merged this on Oct 20, 2015

laanwj closed this on Oct 20, 2015

laanwj referenced this in commit e26a3f6713 on Oct 20, 2015

in src/bitcoin-cli.cpp:None in 55a89751fa

  85 | @@ -88,8 +86,10 @@ static bool AppInitRPC(int argc, char* argv[])
  86 |          return false;
  87 |      }
  88 |      // Check for -testnet or -regtest parameter (BaseParams() calls are only valid after this clause)
  89 | -    if (!SelectBaseParamsFromCommandLine()) {
  90 | -        fprintf(stderr, "Error: Invalid combination of -regtest and -testnet.\n");
  91 | +    try {
  92 | +        SelectBaseParams(ChainNameFromCommandLine());
  93 | +    } catch(std::exception &e) {

Diapolo commented at 7:31 PM on October 21, 2015:

I don't get it, why such stuff is alway reintroduced or even used at all: Should be: catch (const std::exception& e)

jtimon commented at 10:39 AM on October 30, 2015:

My fault. I would have changed it if someone had nit it on time.

in src/bitcoin-tx.cpp:None in 55a89751fa

  34 | @@ -35,8 +35,10 @@ static bool AppInitRawTx(int argc, char* argv[])
  35 |      ParseParameters(argc, argv);
  36 |  
  37 |      // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
  38 | -    if (!SelectParamsFromCommandLine()) {
  39 | -        fprintf(stderr, "Error: Invalid combination of -regtest and -testnet.\n");
  40 | +    try {
  41 | +        SelectParams(ChainNameFromCommandLine());
  42 | +    } catch(std::exception &e) {

Diapolo commented at 7:32 PM on October 21, 2015:

Same here, should be: catch (const std::exception& e)

in src/bitcoind.cpp:None in 55a89751fa

 101 | @@ -102,8 +102,10 @@ bool AppInit(int argc, char* argv[])
 102 |              return false;
 103 |          }
 104 |          // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
 105 | -        if (!SelectParamsFromCommandLine()) {
 106 | -            fprintf(stderr, "Error: Invalid combination of -regtest and -testnet.\n");
 107 | +        try {
 108 | +            SelectParams(ChainNameFromCommandLine());
 109 | +        } catch(std::exception &e) {

Diapolo commented at 7:34 PM on October 21, 2015:

And here: catch (const std::exception& e)

in src/qt/bitcoin.cpp:None in 55a89751fa

 603 | @@ -604,8 +604,10 @@ int main(int argc, char *argv[])
 604 |      // - Needs to be done before createOptionsModel
 605 |  
 606 |      // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
 607 | -    if (!SelectParamsFromCommandLine()) {
 608 | -        QMessageBox::critical(0, QObject::tr("Bitcoin Core"), QObject::tr("Error: Invalid combination of -regtest and -testnet."));
 609 | +    try {
 610 | +        SelectParams(ChainNameFromCommandLine());
 611 | +    } catch(std::exception &e) {

Diapolo commented at 7:35 PM on October 21, 2015:

And here: catch (const std::exception& e)

in src/chainparams.cpp:None in 55a89751fa

 265 | -    switch (network) {
 266 | -        case CBaseChainParams::MAIN:
 267 | +CChainParams& Params(const std::string& chain)
 268 | +{
 269 | +    if (chain == CBaseChainParams::MAIN)
 270 |              return mainParams;

Diapolo commented at 7:39 PM on October 21, 2015:

Nit: These indentations could've been corrected also...

jtimon commented at 10:41 AM on October 30, 2015:

Doing it later in something like #6907 results in less total changes.

zkbot referenced this in commit f1aeaec471 on Mar 21, 2018

zkbot referenced this in commit 4fc490c430 on Dec 4, 2019

zkbot referenced this in commit 868c63f92d on Dec 4, 2019

furszy referenced this in commit ac067073ec on Feb 10, 2021

DrahtBot locked this on Sep 8, 2021