datacarriersize warning with release notes
#33224
Follow-up to #32406
The release notes claim
[…] marked as deprecated and are expected to be removed in a future release
but the warning itself claims
[…] marked as deprecated. They will be removed in a future version.
To be less aggressive (since some have objected against this version online) - and to unify the deprecation warning with the release notes - I have changed the warning to communicate our expectation in a friendlier way.
Unified the deprecation warning for the recently deprecated datacarrier[size] options to match the phrasing of release-notes-32406.md.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33224.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | janb84, Zero-1729, cedwies, jonatack, ryanofsky, hodlinator, ajtowns, w0xlt, kevkevinpal, optout21, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Pr changes warning of datacarriersize to a friendlier one. The friendlier text aligns also with the release notes.
Given that deprecation not always results in removal (in this project), I find this warning message a better representation of the reality.
LGTM
crACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Good catch; the new message tone is more aligned and communicates the intention better.
ACK 2885bd0
The PR adjusts the -datacarrier/-datacarriersize deprecation warning to be less absolute and better match the release notes.
I think the new wording still communicates deprecation, but without overstating certainty about removal.
Code change is minimal and the functional test was updated accordingly.
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Makes wording consistent with release notes (end of line):
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Unless there’s an explicit schedule for the removal (eg -paytxfee is deprecated and will be fully removed in v31.0), this phrasing seems more accurate. Probably the testnet3 deprecation should also either be scheduled or changed to “is expected to be removed” as well.
ACK https://github.com/bitcoin/bitcoin/pull/33224/commits/2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Will the src/qt/bitcoinstrings.cpp file be changed in the GUI repository?
cc @hebasto; given this would change translations after translation string freeze.
The translation workflow on Transifex includes marking translated strings as “Reviewed”, which locks them from further changes. Not every translation team uses this feature, but those who do rely on it. Unfortunately, the “opensource” plan used by the Bitcoin organization on Transifex has very limited functionality, and it is not guaranteed that updating the translation source file (as in #33193) won’t reset the “Reviewed” status for other strings.
In short, there must not be any translation source updates after the final translation string freeze.
ACK 2885bd0
Makes sense to have consistent working with the release note
Unfortunately, the “opensource” plan used by the Bitcoin organization on Transifex has very limited functionality, and it is not guaranteed that updating the translation source file (as in #33193) won’t reset the “Reviewed” status for other strings.
Can we pay them some money to make that no longer a problem?
I’ve done some experimenting with a test project on transifex to see what the actual effects this change would cause, and a potential workaround.
In this test project, I copied over a couple fully translated languages to observe how the translated strings change when the source is updated. After rebasing this PR branch and doing cmake --build build --target translate, I uploaded the resulting modified bitcoin_en.xlf file. This resulted in 122 (~10% of strings) being marked untranslated again in both languages. We can assume that that behavior will appear across all languages, so that means ~10% of all translated strings would need to be retranslated. That’s the crux of the problem. Now, Transifex does provide suggestions based on previous translations so those translations can be filled in with the click of a button. But that means someone needs to do that for every single language. I think this issue is why we institute a translation strings freeze.
However, I think there is a workaround to this issue. Ultimately the reason transifex thinks 121 additional strings need to be translated is because the .xlf files contain an id for each string which is based upon the position of a string in the bitcoin_en.ts file which is automatically generated from the source code using lupdate. But if we instead modify bitcoin_en.ts directly as well, lupdate will not change the string order so the generated bitcoin_en.xlf file will contain just the changed string (and some other autogenerated context changes that don’t seem to make a difference), which can be uploaded to Transifex. The result is that only this single string needs to be translated and no other strings will be affected.
If this doc change is desired by contributors, and the 9 acks suggested that it is, then I think this workaround of modifying bitcoin_en.ts would be suitable.
Ultimately the reason transifex thinks 121 additional strings need to be translated is because the
.xlffiles contain an id for each string which is based upon the position of a string in thebitcoin_en.tsfile which is automatically generated from the source code usinglupdate
Is this context info useful? Line numbers will change whenever some previous function in the file gets a non-trivial edit… Would it be better to add -locations none to the LCONVERT invocation in translate.cmake? (Post branch-off) Or are there instances where the same english text (in the same source file?) gets multiple different translations in a single language?
EDIT: https://wiki.qt.io/Qt_Localization seems to suggest -locations none is a good idea fwiw. Could also consider switching from tranifex to weblate, seems to be what fedora uses; mumble switched with this rationale after this discussion – we’d be under the 10k 160k string limit but over the 60 language limit so would presumably need to pay 500 EUR/year and apparently they now offer unlimited languages so that would also be fine; and we could also self-host which is what fedora does. They seem to offer context links back to the source repo, and some degree of github integration for automating pulls/pushes.
Is this context info useful? Line numbers will change whenever some previous function in the file gets a non-trivial edit… Would it be better to add
-locations noneto the LCONVERT invocation in translate.cmake?
I don’t think line numbers is the problem. The actual source location of the string can change and the ids in the .xlf won’t change. It’s the sort order in the .ts file that lupdate generates that seems to cause the id change. It’s unclear to me why the sort order would change.
I’ve done some experimenting with a test project on transifex to see what the actual effects this change would cause, and a potential workaround.
In this test project, I copied over a couple fully translated languages to observe how the translated strings change when the source is updated. After rebasing this PR branch and doing
cmake --build build --target translate, I uploaded the resulting modifiedbitcoin_en.xlffile. This resulted in 122 (~10% of strings) being marked untranslated again in both languages. We can assume that that behavior will appear across all languages, so that means ~10% of all translated strings would need to be retranslated. That’s the crux of the problem. Now, Transifex does provide suggestions based on previous translations so those translations can be filled in with the click of a button. But that means someone needs to do that for every single language. I think this issue is why we institute a translation strings freeze.
Was “Translation Memory Fillup” enabled then?
If this doc change is desired by contributors, and the 9 acks suggested that it is, then…
… it can be accepted as is, but left untranslated until the version v31.0.
This is acceptable because:
I still hesitant about introducing last-minute changes to the translation pipeline / framework. We can revisit all related suggestions, such as dropping locations and ids, right after branching off.
Was “Translation Memory Fillup” enabled then?
Only if it is by default. I don’t know where that setting is.
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Minute details like this in documentation improve the communication of the project. The change is doc-only, minimal, well isolated.