refactor: Add ParseBool to rpc/util #19544
Conversation
|
#19501 introduces more |
src/rpc/blockchain.cpp
Outdated
| if (!request.params[0].isNull()) | ||
| fVerbose = request.params[0].get_bool(); | ||
|
|
||
| const bool fVerbose = GetBool(request.params[0], false); |
There was a problem hiding this comment.
Doesn't this change behavior when passed an int?
If you want to refactor this to a single line without changing behavior, it would read maybe so:
| const bool fVerbose = GetBool(request.params[0], false); | |
| const bool fVerbose{request.params[0].isNull() ? false : request.params[0].get_bool()}; |
There was a problem hiding this comment.
Yeah, sorry, I forgot to comment on that it would allow us to use ints in more cases. I guess the int case is only used in rawtransaction.cpp for historical reasons and we rather want to error in other cases? I removed the int part from the function and removed its use from the one place where it's relevant. And I know this can be made a one-liner but I don't like reading ternary operators for the same base case because I still feel like I need to make sure there is not something less intuitive going on like with the is_witness stuff, a ! hiding somewhere etc. But if you don't see the same benefit it's fine if we close it, I think just using ternary operators isn't worth the change.
There was a problem hiding this comment.
I like the idea of factoring this out to a function, but I don't personally think ints should be accepted where bools are expected in more cases. Being specific about input types helps catch, for example, bugs when the argument is shifted.
There was a problem hiding this comment.
I agree, I changed that already so there is no behavior change anymore now. I just didn't make a clearer comment on it.
|
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. |
|
Took suggestions by @promag |
The helper function encapsulates the boolean parse logic in one place.
|
Need to update PR title and reword commit. |
Done, also rebased on master to get access to the git commit message linter that failed in the CI but not locally on my one-liner commit message. |
|
ACK c2551d0 -- suggested version is easier to reason about and patch looks correct |
|
Should a helper for every type be added? It would probably be better to hide all parsing etc within RPCHelpMan, so that the logic in rpc methods itself doesn't need to worry about that |
Yeah, makes sense I think. Unless that is something you are already working on, I will explore it and make a suggestion. |
|
I'd suspect this can only be done after #18531 |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Related: #20017 |
Mini refactoring for DRYer code and better readability. Replaces several instances of bool parsing in the RPC code with a rpc/util function. Also uses const when appropriate.