RPC: Consistently use UniValue.pushKV instead of push_back(Pair()) #11386
Conversation
Consistently use UniValue.pushKV instead of UniValue.push_back(Pair()) Replicable by `find . -name '*.cpp' -print0 | xargs -0 sed -i 's/push_back(Pair(\(.*\)));/pushKV(\1);/g'`
|
We have in fact a simple framework for verifiable scripted changes, see for example #10483. Perhaps you can use that for the first commit. |
|
UniValue is added as a git subtree. Please propose changes that affect the UniValue sources upstream (https://github.com/bitcoin-core/univalue). |
|
@jonasschnelli This is changing how UniValue is used, not changing UniValue itself. |
|
Oh, missed that. |
|
@Runn1ng will you remove univalue changes and submit a patch there? |
|
Sorry, busy weekend, I am doing it now |
|
FWIW, jgarzik/univalue#42 |
|
Here is the PR bitcoin-core/univalue-subtree#5 |
|
For now 1st commit is enough. Meanwhile other merged PR's can use Concept ACK. |
|
First commit is not enough.
Without the UniValue KV bool fix, saving bool values through pushKV falls
back to some numeric type (not sure which one now) and is saved wrong in
the json. Some RPCs are using bool values and tests (correctly) fail.
I am waiting for the UniValue PR to be merged, and then I will change this
PR to just be a pull from the UniValue subtree + scripted commit.
…On Sep 27, 2017 5:31 PM, "João Barbosa" ***@***.***> wrote:
For now 1st commit is enough. Meanwhile other merged PR's can use
.push_back(Pair(...)) so keep an eye on that.
Concept ACK.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11386 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGZ8S5eqlqn_XcKfAUhI9zvQoIAdWSmks5smmpVgaJpZM4PhJax>
.
|
|
I know and IMO a failing build is better. Anyway, concept ACK. |
|
I think the subtree update and scripted-diff would need to be atomic to avoid breaking git bisect. Is that correct? Maybe check with @laanwj or @MarcoFalke if that's possible? |
|
If #10583 is merged then there are a couple of |
|
@jnewbery Since the scripted diff can not contain any other changes (like changes to a subtree) I don't think this is possible. However, it is possible to do in one commit when the scripted diff keyword does not appear. |
|
I don't think I can do it both atomically for bisect and as git subtree merge. I think I can make it atomically if I manually add the changes to the subtree files inside the script, but that will be a bit ugly too. |
|
If it's impossible to do at once due to the subtree update being a separate commit, I think it's fine to do it as a separate commit in the same PR. |
|
To keep git bisect working, we'd need to do two commits in univalue:
and do three commits in this repo:
is that possible? |
|
@jnewbery yeah, that could be possible. Actually, the PR in univalue is already written like that. Now I am waiting for the univalue PR to be merged :) |
|
It can be done atomically in two commits. See #11420 |
|
FWIW #11420 was merged, anything left to do here? |
|
bitcoin-core/univalue-subtree#5 needs to be merged in bitcoin-core/univalue before this can go in. |
|
This now requires bitcoin-core/univalue-subtree#10 and bitcoin-core/univalue-subtree#11 to be merged, and then separate subtree bumps for those PRs, with the "Consistently use UniValue.pushKV instead of UniValue.push_back(Pair())" commit between those two subtree bumps. @karel-3d has said that he doesn't have time to maintain these PRs, so I'll open a PR that handles this once the univalue PRs are merged. |
|
bitcoin-core/univalue-subtree#10 is merged. Waiting for bitcoin-core/univalue-subtree#11 before I open a new PR for this. |
|
@jnewbery I think it is cleaner to bump the subtree separately. (Once for bitcoin-core/univalue-subtree#10, and then some time in the future, not necessarily now, for bitcoin-core/univalue-subtree#11) |
Sure, I'm happy to review a PR that does that. |
…ack(Pair()) (karel-3d) 91986ed scripted-diff: Use UniValue.pushKV instead of push_back(Pair()) (Karel Bilek) a570098 Squashed 'src/univalue/' changes from 07947ff2da..51d3ab34ba (MarcoFalke) Pull request description: Rebased version of #11386 by karel-3d. Closes: #11386 Tree-SHA512: f3a81447e573c17e75813f4d41ceb34b9980eac81efdd98ddb149d7c51f792be7e2b32239b6ea7e6da68af23897afa6b4ce3f4e8070f9c4adf5105bf6075f2a0
As I was poking in the code, I didn't like that at some places, pushKV is being used, and at other places, push_back(Pair()) is being used. So I changed the style to pushKV. (It's done automatically by sed, see the git comment on the first commit)
I also realized that the Pair Univalue code is not used anymore with this change anymore, and since there is this comment in the source code for about 2 years
I went ahead and removed it
It's all mostly bikeshed