Remove connect_nodes global and Replace connect_nodes(self.nodes[a], b) with self.connect_nodes(a, b) #19945

1. 
   ghost commented at 8:14 pm on September 11, 2020: none

   Made changes in 37 files based on the issue and solution suggested by MarcoFalke in #19821
2. Remove confusing connect_nodes global

   Replace connect_nodes(self.nodes[a], b) with self.connect_nodes(a, b) and remove the global connect_nodes

   bbbb6242b5
3. DrahtBot added the label Tests on Sep 11, 2020
4. 
   michaelfolkson commented at 10:50 pm on September 11, 2020: contributor

   A few things to note from the contributing guidelines.

   • The contributor workflow is to fork the project and create a separate topic branch (i.e. not open a PR from master)
   • You should use the fixes or closes keywords when referencing the issue it fixes/closes as this will cause the corresponding issue to be closed when the pull request is merged.
   • For find and replace like exercises we use scripted diffs.

   If anything isn’t clear or you need further help let us know.

5. 
   ghost commented at 1:42 pm on September 12, 2020: none

   1. I had created a separate branch for bitcoin-core/gui but I thought master branch can be used for bitcoin/bitcoin

   I can use a different branch in future or delete this if it’s not acceptable.

   2. Agree I could have used a better description. In this case there wasn’t much to describe and I thought it’s better to link the issue which has all the details instead of writing the same thing.

   3. While making the changes I realized it requires manually doing it carefully. So not sure if it could have been done with script.

   4. There were some conflicts during CI checks and trying to resolve them I created few others so completely deleted old PR and created new one.

   5. I will keep the things in mind while submitting a PR in future, let me know if this needs changes/closure or can be merged.

6. 
   michaelfolkson commented at 2:22 pm on September 12, 2020: contributor

   I had created a separate branch for bitcoin-core/gui but I thought master branch can be used for bitcoin/bitcoin

   I would recommend you fork both repositories (bitcoin/bitcoin and bitcoin-core/gui) individually and treat them as separate repos.

   In reality bitcoin/bitcoin pulls in changes made to bitcoin-core/gui but if you continue contributing you will be opening PRs and reviewing PRs on both repositories depending on whether it is GUI or not. Then you can have your master branch follow the master of each repo and set up topic branches off master.

   While making the changes I realized it requires manually doing it carefully. So not sure if it could have been done with script.

   Unless I am missing something this is a find and replace which can be done using a scripted-diff. I don’t think this will be merged without a scripted-diff but someone more experienced can correct me if I am wrong.

   I will keep the things in mind while submitting a PR in future, let me know if this needs changes/closure or can be merged.

   As I said I don’t think this will be merged as is. You’d need to close this PR to open a new PR from a topic branch and provide a scripted-diff. If you need help with the latter please say.

7. fanquake deleted a comment on Sep 14, 2020
8. 
   robot-dreams commented at 8:03 am on September 17, 2020: contributor

   Thanks so much for working on this, @prayank23 !

   I’ve built on your work over in #19967 to use a scripted-diff as suggested, and I kept you as the author for the relevant commit: 1d21af136e67ba25f4734ed077020ffcc761c07b

9. 
   michaelfolkson commented at 9:19 am on September 17, 2020: contributor

   Yeah nice work @prayank23. Hopefully this will be useful for future new contributors.

   Feel free to look at the scripted-diff commit in @robot-dreams PR and if it works, makes sense to you, you can review his PR by Approach ACKing or ACKing the commit.

   Any questions on the scripted-diff can be asked here or in the #bitcoin-core-pr-reviews channel on IRC.

10. 
    DrahtBot commented at 1:38 pm on September 19, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19967 (test: Replace (dis)?connect_nodes globals with TestFramework methods by robot-dreams)
    • #19877 ([test] clarify rpc_net & p2p_disconnect_ban functional tests by amitiuttarwar)
    • #18788 (tests: Update more tests to work with descriptor wallets by achow101)
    • #18766 (Disable fee estimation in blocksonly mode (by removing the fee estimates global) by darosior)

    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.

11. unknown closed this on Sep 19, 2020

    ---

12. MarcoFalke referenced this in commit 47fc883106 on Oct 21, 2020
13. sidhujag referenced this in commit fa2305a135 on Oct 21, 2020
14. DrahtBot locked this on Feb 15, 2022

Contributors
ghost michaelfolkson robot-dreams DrahtBot

Labels
Tests