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

1. 
   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.
2. 
   hebasto commented at 2:33 pm on July 21, 2022: member

   Also #20404.
3. 
   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.

4. DrahtBot added the label Refactoring on Jul 21, 2022
5. 
   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.

6. 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.
7. luke-jr changes_requested
8. DrahtBot added the label Needs rebase on Aug 5, 2022
9. 
   jarolrod commented at 5:22 am on September 20, 2022: member

   concept ack, needs rebase
10. ryanofsky approved
11. 
    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.

12. refactor: wallet, do not translate init arguments names e43a547a36
13. furszy force-pushed on Oct 6, 2022
14. 
    furszy commented at 9:31 pm on October 6, 2022: member

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

    Code review ACK e43a547a3674a31504a60ede9b4912e014a54139. Just rebased since last review.
18. fanquake requested review from hebasto on Oct 13, 2022
19. 
    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/
20. 
    achow101 commented at 10:39 pm on March 17, 2023: member

    ACK e43a547a3674a31504a60ede9b4912e014a54139
21. 
    maflcko commented at 9:34 am on March 18, 2023: member

    lgtm ACK e43a547a3674a31504a60ede9b4912e014a54139
22. fanquake merged this on Mar 19, 2023
23. fanquake closed this on Mar 19, 2023

    ---

24. sidhujag referenced this in commit 81df80487b on Mar 19, 2023
25. 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")};
    

    ---

    ---

    

    flack commented at 10:43 am on March 20, 2023:

    Strictly speaking, the middle sentence is a bit redundant (and also contains the option name)

    ---

    

    hebasto commented at 2:36 pm on April 17, 2023:

    For translators, it will be better to drop the middle sentence or put a place holder instead of the option name.
26. bitcoin locked this on Apr 16, 2024

Contributors
furszy hebasto DrahtBot luke-jr jarolrod ryanofsky achow101 maflcko flack

Labels
Refactoring