refactor: Error message bilingual_str consistency #19176
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Concept ACK |
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Member
|
Concept ACK. |
jonatack
reviewed
Jun 5, 2020
Member
jonatack
left a comment
There was a problem hiding this comment.
ACK f40dddfde231550e76043011d4211ca2fb17143d
hebasto
approved these changes
Jun 5, 2020
Member
hebasto
left a comment
There was a problem hiding this comment.
ACK f40dddfde231550e76043011d4211ca2fb17143d, tested on Linux Mint 19.3 (x86_64).
src/net_permissions.cpp
Outdated
Member
There was a problem hiding this comment.
nit: It seems using an unnamed namespace is preferable:
- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Rs-unnamed2
- https://stackoverflow.com/questions/154469/unnamed-anonymous-namespaces-vs-static-functions
- https://stackoverflow.com/questions/4422507/superiority-of-unnamed-namespace-over-static
- https://stackoverflow.com/questions/4977252/why-an-unnamed-namespace-is-a-superior-alternative-to-static
maflcko
approved these changes
Jun 5, 2020
Member
maflcko
left a comment
There was a problem hiding this comment.
ACK. This should also fix the failing tests when running the net permission tests in the gui
This was referenced Jun 5, 2020
Member
|
Probably unrelated or a bad build, but with two different debug builds on this branch and not on master, I'm seeing seeing this shortly after launching the GUI. Testing further with rebase on master. debug log
|
Member
|
@jonatack This issue has been fixed on master |
Member
|
Thanks, suspected as much. I just verified with a build of this branch rebased on master that the issue is gone. |
This was referenced Jun 5, 2020
- Move the decision whether to translate an error message to where it is defined. This simplifies call sites: no more `InitError(Untranslated(...))`. - Make all functions in `util/error.h` consistently return a `bilingual_str`. We've decided to use this as error message type so let's roll with it. This has no functional changes: no messages are changed, no new translation messages are defined.
It's only used inside `net_permissions.cpp`.
Make it consistent with `Chain::initError`.
f40dddf to
6fe9890
Compare
Member
Author
|
Member
|
Tested before: Tested after: ACK 6fe9890 🔣 Show signature and timestampSignature: Timestamp of file with hash |
Member
Trying to reproduce your tests, and got: What am I doing wrong? |
Member
You'll need to manually close the GUI dialogs. The python test framework can't do that for you (yet). |
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
Jun 10, 2020
After bitcoin#19176, building the gui on Bionic is failing with: ```bash CXX qt/qt_libbitcoinqt_a-guiutil.o qt/bitcoin.cpp: In function 'int GuiMain(int, char**)': qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error)); ``` The merge commit also failed to compile with the same error: https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 10, 2020
6fe9890 refactor: Change Node::initError to take bilingual_str (Wladimir J. van der Laan) 425e7cb refactor: Put`TryParsePermissionFlags` in anonymous namespace (Wladimir J. van der Laan) 77b79fa refactor: Error message bilingual_str consistency (Wladimir J. van der Laan) Pull request description: A straightforward and hopefully uncontroversial refactor to improve consistency. - Move the decision whether to translate an individual error message to where it is defined. This simplifies call sites: no more `InitError(Untranslated(SomeFunction(...)))`. - Make all functions in `util/error.h` consistently return a `bilingual_str`. We've decided to use this as error message type so let's roll with it. This has no functional changes: no messages are changed, no new translation messages are defined. Also make a function static that can be static. ACKs for top commit: MarcoFalke: ACK 6fe9890 🔣 hebasto: ACK 6fe9890, tested on Linux Mint 19.3 (x86_64). Tree-SHA512: 1dd123ef285c4b50bbc429b2f11c9a63aaa669a84955a0a9b8134e9dc141bc38f863f798e8982ac68bbe83170e1067a87d1a87fe7f791928b7914e10bbc2ef8d
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 10, 2020
…nsistency" This reverts commit d154202.
fanquake
added a commit
that referenced
this pull request
Jun 10, 2020
948f113 gui: add missing translation.h include to fix build (fanquake) Pull request description: After #19176, building the gui on Bionic is failing with: ```bash CXX qt/qt_libbitcoinqt_a-guiutil.o qt/bitcoin.cpp: In function 'int GuiMain(int, char**)': qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error)); ``` The merge commit also failed to compile with the same error: https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543 ACKs for top commit: hebasto: ACK 948f113, tested on Linux Mint 19.3 (x86_64): it fixes compiling error with the `--disable-wallet` configure option. Tree-SHA512: db0197b110b3a7d05af2ceb29fbe9eeb6521d28f53b6267aa6d07a975886adb5c6485af79506ab6c66ed101e32292feeaff3707cdbc11432e5b97400953d5631
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 10, 2020
After bitcoin#19176, building the gui on Bionic is failing with: ```bash CXX qt/qt_libbitcoinqt_a-guiutil.o qt/bitcoin.cpp: In function 'int GuiMain(int, char**)': qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error)); ``` The merge commit also failed to compile with the same error: https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
stackman27
pushed a commit
to stackman27/bitcoin
that referenced
this pull request
Jul 4, 2020
After bitcoin#19176, building the gui on Bionic is failing with: ```bash CXX qt/qt_libbitcoinqt_a-guiutil.o qt/bitcoin.cpp: In function 'int GuiMain(int, char**)': qt/bitcoin.cpp:460:35: error: 'Untranslated' was not declared in this scope node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error)); ``` The merge commit also failed to compile with the same error: https://travis-ci.org/github/bitcoin/bitcoin/jobs/696627543
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 14, 2020
Summary:
```
A straightforward and hopefully uncontroversial refactor to improve
consistency.
Move the decision whether to translate an individual error message
to where it is defined. This simplifies call sites: no more
InitError(Untranslated(SomeFunction(...))).
Make all functions in util/error.h consistently return a
bilingual_str. We've decided to use this as error message type so let's
roll with it.
This has no functional changes: no messages are changed, no new
translation messages are defined.
Also make a function static that can be static.
```
Backport of core [[bitcoin/bitcoin#19176 | PR19176]].
The change to `kitchen_sink.cpp` is not ported since this file hasn't
been ported yet, but the compiler will make the change obvious when
needed.
Depends on D8650.
Test Plan:
ninja all check-all
ninja bitcoin-fuzzers
Reviewers: #bitcoin_abc, PiRK
Reviewed By: #bitcoin_abc, PiRK
Differential Revision: https://reviews.bitcoinabc.org/D8651
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A straightforward and hopefully uncontroversial refactor to improve consistency.
Move the decision whether to translate an individual error message to where it is defined. This simplifies call sites: no more
InitError(Untranslated(SomeFunction(...))).Make all functions in
util/error.hconsistently return abilingual_str. We've decided to use this as error message type so let's roll with it.This has no functional changes: no messages are changed, no new translation messages are defined.
Also make a function static that can be static.