refactor: wallet, do not translate init options names #25666
Conversation
|
Also #20404. |
|
hmm k thanks @hebasto, I do agree with laanwj there. Some contextual information wouldn't be bad for this. 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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
This message should probably be deduplicated to one location.
There was a problem hiding this comment.
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.
|
concept ack, needs rebase |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
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.
furszy
force-pushed
the
2022_refactor_do_not_translate_init_flags
branch
from
5fb8b86 to
e43a547
Compare
|
great to see some movement here, rebased. |
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
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/
|
ACK e43a547 |
|
lgtm ACK e43a547 |
…names e43a547 refactor: wallet, do not translate init arguments names (furszy) Pull request description: 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. ACKs for top commit: achow101: ACK e43a547 MarcoFalke: lgtm ACK e43a547 ryanofsky: Code review ACK e43a547. Just rebased since last review. Tree-SHA512: c6eca98fd66d54d5510de03ab4e63c00ba2838af4237d2bb135d01c47f8ad8ca9aa7ae1e45cf668afcfb9dd958b075a1756cc887b3beef2cb494933d4d83eab0
| if (feeCalc.reason == FeeReason::FALLBACK && !wallet.m_allow_fallback_fee) { | ||
| // eventually allow a fallback fee | ||
| return util::Error{_("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.")}; | ||
| return util::Error{strprintf(_("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable %s."), "-fallbackfee")}; |
There was a problem hiding this comment.
Strictly speaking, the middle sentence is a bit redundant (and also contains the option name)
There was a problem hiding this comment.
For translators, it will be better to drop the middle sentence or put a place holder instead of the option name.
10 participants
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.