RPC: Consistently use UniValue.pushKV instead of push_back(Pair()) #11386

1. 
   karelbilek commented at 7:37 pm on September 22, 2017: contributor

   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

   // Most duplicate other methods, and should be removed

   I went ahead and removed it

   It’s all mostly bikeshed

2. Consistently use UniValue.pushKV

   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'`

   69f623828c
3. Pushing boolean value to univalue correctly 7e79fd2a8a
4. Removing unused Pair from univalue 7cf7690248
5. 
   sipa commented at 7:44 pm on September 22, 2017: member

   We have in fact a simple framework for verifiable scripted changes, see for example #10483. Perhaps you can use that for the first commit.
6. 
   jonasschnelli commented at 8:31 pm on September 22, 2017: contributor

   UniValue is added as a git subtree. Please propose changes that affect the UniValue sources upstream (https://github.com/bitcoin-core/univalue).
7. jonasschnelli added the label Refactoring on Sep 22, 2017
8. 
   achow101 commented at 9:09 pm on September 22, 2017: member

   @jonasschnelli This is changing how UniValue is used, not changing UniValue itself.
9. 
   jonasschnelli commented at 9:10 pm on September 22, 2017: contributor

   @achow101: what about that https://github.com/bitcoin/bitcoin/pull/11386/files#diff-49e8bd958c3337da3e89c4a731dc37fd?
10. 
    achow101 commented at 9:42 pm on September 22, 2017: member

    Oh, missed that.
11. dcousens approved
12. 
    promag commented at 9:07 am on September 24, 2017: member

    @runn1ng will you remove univalue changes and submit a patch there?
13. 
    karelbilek commented at 9:26 pm on September 24, 2017: contributor

    Sorry, busy weekend, I am doing it now
14. 
    jgarzik commented at 9:41 pm on September 24, 2017: contributor

    FWIW, https://github.com/jgarzik/univalue/pull/42
15. 
    karelbilek commented at 9:57 pm on September 24, 2017: contributor

    Here is the PR https://github.com/bitcoin-core/univalue/pull/5
16. 
    promag commented at 3:29 pm on September 27, 2017: member

    For now 1st commit is enough. Meanwhile other merged PR’s can use .push_back(Pair(...)) so keep an eye on that.

    Concept ACK.

17. 
    karelbilek commented at 3:40 pm on September 27, 2017: contributor

    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” notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/11386#issuecomment-332560270, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGZ8S5eqlqn_XcKfAUhI9zvQoIAdWSmks5smmpVgaJpZM4PhJax .

18. 
    promag commented at 4:05 pm on September 27, 2017: member

    I know and IMO a failing build is better. Anyway, concept ACK.
19. 
    jnewbery commented at 9:01 pm on September 27, 2017: member

    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?

20. 
    promag commented at 8:51 am on September 29, 2017: member

    If #10583 is merged then there are a couple of push_back to replace after rebase.
21. 
    MarcoFalke commented at 12:45 pm on September 29, 2017: member

    @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.
22. 
    karelbilek commented at 9:52 am on September 30, 2017: contributor

    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.

23. 
    sipa commented at 6:13 pm on September 30, 2017: member

    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.
24. 
    jnewbery commented at 1:18 am on October 1, 2017: member

    To keep git bisect working, we’d need to do two commits in univalue:

    1. Pushing boolean value to univalue correctly
    2. Remove deprecated std::pair wrappers

    and do three commits in this repo:

    1. subtree update, pulling commit (1) from univalue
    2. do the scripted-diff to change from push_back(Pair()) to pushKV()
    3. subtree update, pulling commit (2) from univalue

    is that possible?

25. 
    karelbilek commented at 3:36 pm on October 12, 2017: contributor

    @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 :)

26. 
    MarcoFalke commented at 6:34 pm on November 10, 2017: member

    It can be done atomically in two commits. See #11420
27. 
    laanwj commented at 12:59 pm on December 12, 2017: member

    FWIW #11420 was merged, anything left to do here?
28. 
    jnewbery commented at 6:21 pm on December 12, 2017: member

    https://github.com/bitcoin-core/univalue/pull/5 needs to be merged in bitcoin-core/univalue before this can go in.
29. 
    jnewbery commented at 6:16 pm on January 8, 2018: member

    This now requires https://github.com/bitcoin-core/univalue/pull/10 and https://github.com/bitcoin-core/univalue/pull/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.
30. 
    jnewbery commented at 2:20 pm on January 15, 2018: member

    https://github.com/bitcoin-core/univalue/pull/10 is merged. Waiting for https://github.com/bitcoin-core/univalue/pull/11 before I open a new PR for this.
31. 
    MarcoFalke commented at 3:21 pm on January 15, 2018: member

    @jnewbery I think it is cleaner to bump the subtree separately. (Once for bitcoin-core/univalue#10, and then some time in the future, not necessarily now, for bitcoin-core/univalue#11)
32. 
    jnewbery commented at 3:28 pm on January 15, 2018: member

    I think it is cleaner to bump the subtree separately.

    Sure, I’m happy to review a PR that does that.

33. MarcoFalke closed this on Feb 12, 2018

    ---

34. MarcoFalke referenced this in commit 5dc00f68c4 on Feb 12, 2018
35. DrahtBot locked this on Sep 8, 2021

Contributors
karelbilek sipa jonasschnelli achow101 dcousens promag jgarzik jnewbery MarcoFalke laanwj

Labels
Refactoring