-
Notifications
You must be signed in to change notification settings - Fork 37.7k
scripted-diff: prefix [address|change]type parameters with 'default' #12216
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
Conversation
Making it clear that these parameters can be overridden by individual wallet commands. -BEGIN VERIFY SCRIPT- sed -i 's/addresstype/defaultaddresstype/g' doc/*.md src/wallet/*.cpp test/functional/*.py sed -i 's/changetype/defaultchangetype/g' doc/*.md src/wallet/*.cpp test/functional/*.py -END VERIFY SCRIPT-
a71d991
to
e47af54
Compare
@MarcoFalke this changes more than just docs, probably needs a wallet and RPC label. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK e47af54. I think this is nice to make it clear that these options won't somehow cause the bitcoin node to dictate an address type.
@@ -16,8 +16,8 @@ | |||
std::string GetWalletHelpString(bool showDebug) | |||
{ | |||
std::string strUsage = HelpMessageGroup(_("Wallet options:")); | |||
strUsage += HelpMessageOpt("-addresstype", strprintf(_("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather improve the description.
BTW, currently RPC help is very clear:
bitcoin/src/wallet/rpcwallet.cpp
Line 147 in 898f560
"2. \"address_type\" (string, optional) The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\". Default is set by -addresstype.\n" |
Reasons for rename: 1) Default value is `<datadir>/wallets` so calling it `-walletsdir` instead of `-walletdir` would be more internally consistent 2) Directory can contain more than one wallet so plural makes more sense 3) This makes it harder to confuse -walletdir option with -wallet option if we store wallets in their own directories as proposed in bitcoin#11466 (comment) and implemented in bitcoin#12216 -BEGIN VERIFY SCRIPT- git grep -l walletdir | xargs sed -i s/walletdir/walletsdir/g -END VERIFY SCRIPT-
Removing this from 0.16 milestone - during the IRC meeting it was found that many options apply to defaults, so adding |
Should be closed then because changing the arguments after a release is a no-go. |
Agree with the above, as well as @laanwj's point "shorter option names are easier to remember/type". The behavior of these parameters did lead to some confusion during code review, but with the proper docs hopefully it will be fine for users. |
Making it clear that these parameters can be overridden by individual wallet commands.
Requesting 0.16 tag, as it's probably not worth changing otherwise.
This makes grateful use of the fact that @sipa used an underscore for
address_type
andchange_type
RPC arguments.