Chainparams: Translations: DRY: options and error strings #6235
pull jtimon wants to merge 2 commits into bitcoin:master from jtimon:chainparams-dry changing 11 files +79 −90-
jtimon commented at 8:02 pm on June 4, 2015: contributorMake 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: contributorNeeded rebase. Also replaced GetParamsHelpMessages with AppendParamsHelpMessages, which optionally takes a showDebugHelp boolean.
-
jtimon force-pushed on Jun 21, 2015
-
jtimon commented at 9:25 pm on June 25, 2015: contributorClosing 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: contributorGreat. MAX_NETWORK_TYPES is removed in the second commit.
-
rustyrussell commented at 6:24 am on July 7, 2015: contributorAck (tested).
-
in src/chainparams.cpp: 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 force-pushed on Aug 21, 2015
-
jtimon commented at 11:35 pm on August 21, 2015: contributorRebased and fixed (see #6077 (comment) ).
-
in src/chainparamsbase.cpp: 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 0:17 am on August 26, 2015:This last line is outdated. That behaviour moved to the separategenerate
RPC call. -
sipa commented at 0:32 am on August 26, 2015: memberUntested ACK
-
theuni commented at 6:25 pm on August 26, 2015: memberut ACK
-
jtimon force-pushed on Aug 26, 2015
-
jgarzik commented at 3:14 pm on September 16, 2015: contributorut 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: contributorutACK
-
jtimon commented at 12:47 pm on September 22, 2015: contributorping
-
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: 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: memberut ACK
-
in src/chainparamsbase.cpp: 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:
-
Chainparams: Replace CBaseChainParams::Network enum with string constants (suggested by Wladimir) f3525e24e3
-
Chainparams: Translations: DRY: options and error strings
Also remove SelectBaseParamsFromCommandLine and SelectParamsFromCommandLine
-
jtimon force-pushed on Oct 20, 2015
-
jtimon commented at 12:31 pm on October 20, 2015: contributorUpdated 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: 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: 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: 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: 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: 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…
-
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