wallet: Fix already-loading message grammar #21040

pull kootoopas wants to merge 1 commits into bitcoin:master from kootoopas:fix-wallet-being-loaded-error-message changing 2 files +2 −2

kootoopas commented at 1:02 PM on January 31, 2021: contributor

No description provided.
fanquake added the label Wallet on Jan 31, 2021
in src/wallet/wallet.cpp:238 in fce0ac2d85 outdated
```
 234 | @@ -235,7 +235,7 @@ std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string&
 235 |  {
 236 |      auto result = WITH_LOCK(g_loading_wallet_mutex, return g_loading_wallet_set.insert(name));
 237 |      if (!result.second) {
 238 | -        error = Untranslated("Wallet already being loading.");
 239 | +        error = Untranslated("Wallet is already currently being loaded.");
```
jonatack commented at 7:39 PM on January 31, 2021:

"Wallet already loading" seems simplest

error = Untranslated("Wallet already loading");
kootoopas commented at 8:18 PM on January 31, 2021:

"being loaded" is more precise since "loading" implies that the wallet is acting instead of being acted on, or at least it introduces the possibility of it acting. So probably "Wallet already being loaded" is the best fit.

laanwj commented at 10:23 AM on February 1, 2021:

I tend to agree with @jonatack here. Shorter (and more direct) is better for error messages.
jonatack commented at 7:42 PM on January 31, 2021: member

Concept ACK. This was added in b9971ae5853c1d62e09d976a8 and 9b009fae6e2, pinging @promag for confirmation on desired meaning.
theStack commented at 8:18 PM on January 31, 2021: member

Concept ACK. Note that the string change for both the wallet and the functional test should ideally happen in the same commit (i.e. the commit should be squashed), otherwise we have a state in-between where the test would fail.
kootoopas force-pushed on Jan 31, 2021
promag commented at 8:39 PM on January 31, 2021: member

This might break someone's code right?
practicalswift commented at 9:14 PM on January 31, 2021: contributor

Concept ACK

Welcome as a contributor @kootoopas!
laanwj commented at 10:24 AM on February 1, 2021: member

This might break someone's code right?

We should be explicit in that error messages are not an interface. If you need to distinguish between errors programmatically they should have a distinct error code. I did this in #20964, for example.

Edit: speaking of which Is "wallet already loading" close enough to "wallet already loaded" to share the same error code?
kootoopas force-pushed on Feb 1, 2021
kootoopas renamed this:
~~wallet: Fix already-being-loaded message grammar~~
wallet: Fix already-loading message grammar
on Feb 1, 2021
jonatack commented at 9:09 PM on February 1, 2021: member

Is "wallet already loading" close enough to "wallet already loaded" to share the same error code?

Good point. Wallets can sometimes take a while to load, so I'm not sure.
wallet: Fix already-loading error message grammar ae9d26a8f0
kootoopas force-pushed on Feb 3, 2021
RandyMcMillan commented at 2:40 PM on February 5, 2021: contributor

...loaded is past tense - which should indicate that the wallet was not corrupted and loaded successfully - from a debugging perspective.

...loading is present tense - if there is an issue in this stage - we can't eliminate the possibility that the wallet isn't corrupted.

In a troubleshooting decision tree - "loaded" assumes the wallet is valid - "loading" doesn't assume the wallet is valid. There should be a distinction in logging.
practicalswift commented at 10:51 PM on March 20, 2021: contributor

cr ACK ae9d26a8f0435e2f4b39ad1181473e6575ac67b5
unknown approved
unknown commented at 5:35 AM on March 21, 2021: none

ACK https://github.com/bitcoin/bitcoin/pull/21040/commits/ae9d26a8f0435e2f4b39ad1181473e6575ac67b5

Will be similar to Bitcoin Core is probably already running https://github.com/bitcoin/bitcoin/blob/63952f73b3041468fe3b25fa54858db7899273fa/src/init.cpp#L1230
MarcoFalke merged this on Mar 21, 2021
MarcoFalke closed this on Mar 21, 2021
kootoopas deleted the branch on Mar 21, 2021
sidhujag referenced this in commit 1fd48ae5f7 on Mar 21, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels