refactor: wallet, do not translate init options names #25666

pull furszy wants to merge 1 commits into bitcoin:master from furszy:2022_refactor_do_not_translate_init_flags changing 2 files +8 −8

furszy commented at 2:18 PM on July 21, 2022: member

Simple, and not interesting, refactor that someone has to do sooner or later. We are translating some init arguments names when those shouldn't be translated.
hebasto commented at 2:33 PM on July 21, 2022: member

Also #20404.
furszy commented at 2:50 PM on July 21, 2022: member

hmm k thanks @hebasto, I do agree with laanwj there. Some contextual information wouldn't be bad for this. We could change a bit the error description by adding something like "invalid init argument value for %s bla bla" so translators can infer what will be placed in %s better.

Saying that, I'm good with any approach here. Pushed it merely because found it while was working on other things in the wallet.

Also, I would keep this PR scoped to the wallet module only (same as someone could tackle other points of #20404 scoped to other area of the sources), mainly to not end up conflicting with many PRs. Otherwise, the rebase work burden would be, by far, bigger than what it costed to do this tiny changes.
DrahtBot added the label Refactoring on Jul 21, 2022
DrahtBot commented at 6:23 PM on July 21, 2022: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK ryanofsky, achow101, MarcoFalke

Concept ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
DrahtBot cross-referenced this on Jul 21, 2022 from issue refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky

Type	Reviewers
ACK	ryanofsky, achow101, MarcoFalke
Concept ACK	hebasto

in src/wallet/wallet.cpp:2953 in 5fb8b862f4 outdated

2908 | @@ -2909,7 +2909,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2909 |      if (args.IsArgSet("-discardfee")) {
2910 |          std::optional<CAmount> discard_fee = ParseMoney(args.GetArg("-discardfee", ""));
2911 |          if (!discard_fee) {
2912 | -            error = strprintf(_("Invalid amount for -discardfee=<amount>: '%s'"), args.GetArg("-discardfee", ""));
2913 | +            error = strprintf(_("Invalid amount for %s=<amount>: '%s'"), "-discardfee", args.GetArg("-discardfee", ""));

luke-jr commented at 2:06 AM on July 26, 2022:

This message should probably be deduplicated to one location.

furszy commented at 9:12 PM on October 6, 2022:

yeah, probably we should first move all the init args reads and validations to a separate file (wallet_init.cpp or something similar). Then cleanup all the string messages at once. So we don't continue mixing init stuff with wallet's internals.

luke-jr changes_requested
DrahtBot cross-referenced this on Jul 27, 2022 from issue refactor: Use util::Result class for wallet loading by ryanofsky
DrahtBot cross-referenced this on Jul 27, 2022 from issue refactor: Replace BResult with util::Result by ryanofsky
DrahtBot added the label Needs rebase on Aug 5, 2022
jarolrod commented at 5:22 AM on September 20, 2022: member

concept ack, needs rebase
ryanofsky approved
ryanofsky commented at 6:27 PM on October 6, 2022: contributor

Code review ACK 5fb8b862f455190dede1253dfd36cec7df14a7a8

There was some concern in #20404 (comment) that this change could lead to more questions from translators. But this PR is more limited than #20404, so this should be an easier place to start. Also it should be possible to provide context to translators through comments, and discussion in #20404 seems not to acknowledge benefits of the change for preventing translation errors.
refactor: wallet, do not translate init arguments names e43a547a36
furszy force-pushed on Oct 6, 2022
furszy commented at 9:31 PM on October 6, 2022: member

great to see some movement here, rebased.
DrahtBot removed the label Needs rebase on Oct 6, 2022
ryanofsky approved
ryanofsky commented at 8:37 PM on October 12, 2022: contributor

Code review ACK e43a547a3674a31504a60ede9b4912e014a54139. Just rebased since last review.
fanquake requested review from hebasto on Oct 13, 2022
hebasto commented at 9:47 AM on October 17, 2022: member
Concept ACK on a general idea. Having some concerns about the approach, and wondering whether Transifex custom placeholders could be a better one.

See:
- https://docs.transifex.com/translation-checks/setting-translation-checks
- https://www.transifex.com/blog/2018/introducing-multiple-custom-placeholders-and-some-editor-goodies-for-translators/
achow101 commented at 10:39 PM on March 17, 2023: member

ACK e43a547a3674a31504a60ede9b4912e014a54139
maflcko commented at 9:34 AM on March 18, 2023: member

lgtm ACK e43a547a3674a31504a60ede9b4912e014a54139
fanquake merged this on Mar 19, 2023
fanquake closed this on Mar 19, 2023
sidhujag referenced this in commit 81df80487b on Mar 19, 2023

in src/wallet/spend.cpp:852 in e43a547a36

 848 | @@ -849,7 +849,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
 849 |      }
 850 |      if (feeCalc.reason == FeeReason::FALLBACK && !wallet.m_allow_fallback_fee) {
 851 |          // eventually allow a fallback fee
 852 | -        return util::Error{_("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.")};
 853 | +        return util::Error{strprintf(_("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable %s."), "-fallbackfee")};