Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 11, 2020 •
edited by fanquake
Loading

Made changes in 37 files based on the issue and solution suggested by MarcoFalke in #19821

Remove confusing connect_nodes global
bbbb624 
Replace connect_nodes(self.nodes[a], b) with self.connect_nodes(a, b) and remove the global connect_nodes
@ghost ghost mentioned this pull request Sep 11, 2020
Closed
@DrahtBot DrahtBot added the Tests label Sep 11, 2020
@michaelfolkson
Copy link

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.

@ghost
Copy link
Author

ghost commented Sep 12, 2020 •
edited by ghost
Loading

  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.

  1. 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.

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

  3. 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.

  4. 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.

@michaelfolkson
Copy link

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.

@bitcoin bitcoin deleted a comment from khendri Sep 14, 2020
@robot-dreams robot-dreams mentioned this pull request Sep 17, 2020
Merged
@robot-dreams
Copy link
Contributor

robot-dreams commented Sep 17, 2020 •
edited
Loading

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: 1d21af1

@robot-dreams robot-dreams mentioned this pull request Sep 17, 2020
Closed
@michaelfolkson
Copy link

michaelfolkson commented Sep 17, 2020 •
edited
Loading

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.

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, 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.

@DrahtBot DrahtBot mentioned this pull request Sep 19, 2020
Merged
@ghost ghost closed this Sep 19, 2020
This was referenced Sep 19, 2020
Merged
Merged
maflcko pushed a commit that referenced this pull request Oct 21, 2020
Merge #19967: test: Replace (dis)?connect_nodes globals with TestFram…
47fc883 
…ework methods

3c7d9ab test: Move (dis)?connect_nodes globals into TestFramework as helpers (Elliott Jin)
4b16c61 scripted-diff: test: Replace uses of (dis)?connect_nodes global (Prayank)
be38684 test: Replace use of (dis)?connect_nodes globals (Elliott Jin)

Pull request description:

  `util.py` defines global helper functions `connect_nodes` and `disconnect_nodes`; however, these functions are confusing because they take a node and an index (instead of two indexes).

  The `TestFramework` object has enough context to convert from `i` to `self.nodes[i]`, so we can replace all instances of `connect_nodes(self.nodes[a], b)` with `self.connect_nodes(a, b)`. Similarly, we can replace instances of `disconnect_nodes`.

  The approach taken in this PR builds on #19945 but uses a scripted-diff for the majority of the changes.

  Fixes: #19821

ACKs for top commit:
  MarcoFalke:
    ACK 3c7d9ab
  guggero:
    ACK 3c7d9ab

Tree-SHA512: e027092748602904abcd986d7299624c8754c3236314b6d8e392e306741c212f266c2207e385adfb194f67ae6559a585ee7b15d639b1d65c4651dbf503e5931a
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2020
Merge bitcoin#19967: test: Replace (dis)?connect_nodes globals with T…
fa2305a 
…estFramework methods

3c7d9ab test: Move (dis)?connect_nodes globals into TestFramework as helpers (Elliott Jin)
4b16c61 scripted-diff: test: Replace uses of (dis)?connect_nodes global (Prayank)
be38684 test: Replace use of (dis)?connect_nodes globals (Elliott Jin)

Pull request description:

  `util.py` defines global helper functions `connect_nodes` and `disconnect_nodes`; however, these functions are confusing because they take a node and an index (instead of two indexes).

  The `TestFramework` object has enough context to convert from `i` to `self.nodes[i]`, so we can replace all instances of `connect_nodes(self.nodes[a], b)` with `self.connect_nodes(a, b)`. Similarly, we can replace instances of `disconnect_nodes`.

  The approach taken in this PR builds on bitcoin#19945 but uses a scripted-diff for the majority of the changes.

  Fixes: bitcoin#19821

ACKs for top commit:
  MarcoFalke:
    ACK 3c7d9ab
  guggero:
    ACK 3c7d9ab

Tree-SHA512: e027092748602904abcd986d7299624c8754c3236314b6d8e392e306741c212f266c2207e385adfb194f67ae6559a585ee7b15d639b1d65c4651dbf503e5931a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michaelfolkson @robot-dreams @DrahtBot