Add pushKV(key, boolean) function (replaces #5) #10
Conversation
include/univalue.h
Outdated
| // The following were added for compatibility with json_spirit. | ||
| // Most duplicate other methods, and should be removed. | ||
| // | ||
| static inline std::pair<std::string,UniValue> Pair(const char *cKey, const char *cVal) |
There was a problem hiding this comment.
Seems fine to remove in principle, but aren't we using these currently?
https://github.com/bitcoin/bitcoin/blob/cdd6bbf10a818d243f973f628e465cc1df98691a/src/rpc/blockchain.cpp#L79-L106? There and in other places.
There was a problem hiding this comment.
Indeed. That code is updated to no longer use push_back(Pair(... in bitcoin/bitcoin#11386 (which is what prompted PR #5)
There was a problem hiding this comment.
It looks like we might need a replacement for bitcoin/bitcoin#11386, since @karel-3d didn't have enough time to maintain #5 (comment), and that's a smaller PR.
There was a problem hiding this comment.
Also it looks like the dependency between bitcoin/bitcoin#11386 is currently circular. 11386 depends on the "Pushing boolean value to univalue correctly" commit in this PR. And the "Remove deprecated std::pair wrappers" commit in this PR depends on 11386. Maybe should drop the "Remove deprecated std::pair wrappers" commit here this PR can be merged, and then the pushKV changes in 11386 can then be merged, and then the Pair code can be removed later.
There was a problem hiding this comment.
looks like the dependency between bitcoin/bitcoin#11386 is currently circular
Can we do the following:
- merge this PR into bitcoin-core/univalue
- Add two new subtree merge commits to RPC: Consistently use UniValue.pushKV instead of push_back(Pair()) bitcoin/bitcoin#11386. One that takes the first commit from this PR (as the first commit in 11386), and one that takes the remaining commits from this PR (as the final commits in 11386)
It looks like we might need a replacement for bitcoin/bitcoin#11386
I'm happy to do that if we can get this PR merged
There was a problem hiding this comment.
Can we do the following
It wouldn't be my preference to partially merge pieces of a univalue PR in a subtree bump PR, because it would require dropping PR metadata (see bitcoin/bitcoin#11952 (comment)), but you could do this. I'd prefer to just deprecate more gently and drop the "Remove deprecated" commit here.
jnewbery
changed the title
|
Reducing the scope of this PR to just add the pushKV(key, bool) function. Removal of the std::pair wrappers is now in #11 |
|
utACK 129bad9 |
|
@laanwj This seems ready for merge |
129bad9 [tests] test pushKV for boolean values (John Newbery) b3c44c9 Pushing boolean value to univalue correctly (Karel Bilek) Pull request description: Adds the new pushKV(key, boolean) function from #5, with test cases. Removal of srd::pair wrappers is in a future PR. Tree-SHA512: d13b377365181723c06d4e4d7efa88f3cdbc6ca06ec2effe2ddf834bdfc7e602072c957296370323cb4e921a6934d55b040ebbdacca1dbff66e75d32073e6ac8
Adds the new pushKV(key, boolean) function from #5, with test cases.
Removal of srd::pair wrappers is in a future PR.