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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

ACK d98c39a

NACK

While this isn't incorrect, it's not meaningfully helpful. It doesn't really add anything useful to the codebase. The code as it is now is not broken nor necessarily wrong either.

Yes, it could be a problem if the test is removed, but it's unlikely that the test will be removed, and if it is removed, the line can be just as easily moved at that time.

PRs like this just end up being noise both in terms of open PRs, and in code churn. While this probably won't cause anything to need to be rebased, that doesn't mean that we should be shuffling things around just to make the code look nicer. Having random refactors like this also breaks git blame which we want to avoid so that it's easier to figure out original rationale for a change.

Closing for now due to controversy.

@achow101

Yes, it could be a problem if the test is removed, but it's unlikely that the test will be removed, and if it is removed, the line can be just as easily moved at that time.

I did not mean to imply that a test might be removed. I was only pointing out that this kind of coding practice is confusing because the variable definition doesn't belong only to the test.

While this isn't incorrect, it's not meaningfully helpful. It doesn't really add anything useful to the codebase. The code as it is now is not broken nor necessarily wrong either.

I disagree. As someone new to the code base trying to make heads or tails of what's going on is confusing. To make it easier for others to contribute, I think I high level of code quality should be a priority.

PRs like this just end up being noise both in terms of open PRs, and i..

While I don't disagree that this PR is very tiny and somewhat pedantic because it's a 1 line change. To me the only argument that makes sense for not including changes like this is that it's tiny. The reason I like tiny PRs is:

1. that I'm still relatively new (especially in terms of time I have to contribute as a unpaid volunteer), so I'd rather start contributing small changes instead of giant ones.

2. Contributing encourages not making super large PRs

3. Tiny PRs like this in my mind should be reviewed and either closed or merged in a short period since it's trivial to review. I'm surprised it's taken 6 months to even be looked at. The time it's taken to get any feedback on a number of PR's I made 6 months ago has changed my focus away from core to other projects for now.

So, While I disagree that this PR isn't meaningful, I do agree it is tiny and as you say causes problems since it's such a small nit pick. Do you have recommendations for how this might be included as a larger PR or grouped with other changes? You're the expert on this area of the codebase and some meaningful feedback for how I can contribute meaningful changes would be appreciated.

@MarcoFalke personally I'd preferred to have left this open until we could have more conversation about it. It was open for 6 months with no feedback and then closed in 2 days without much meaningful dialog imo.

Tiny PRs like this in my mind should be reviewed and either closed or merged in a short period since it's trivial to review. I'm surprised it's taken 6 months to even be looked at.

The fact that a tiny PR like this one has taken 6 months for anyone to even look at it is why I think it is noise. There are over 300 other PRs to look at, with several large projects going on at the same time. People are busy looking at other things so trivial changes refactors such as this are being ignored because no one has the time to look at it. Since this PR does exist, it also means that people looking for things to review may spend a bit of time looking at this briefly or otherwise find it just a bit harder to find a PR that they do want to review since this will be in the PR lists. Those that do look at this will have spent their time on a trivial change when that time that could have been spent reviewing any of the other 300+ PRs. Even though the change is trivial (after reviewing it), it still needs to be reviewed and reviewers need to spend time on this instead of spending time on the other things that the project has collectively decided is useful and desired.

In addition to the time opportunity cost this incurs, it can also have an effect on other, more substantial, PRs. Although git does not detect a merge conflict here, it's possible that this results in a silent merge conflict with another PR. This would result in that PR having to be rebased, which then requires reviewers to re-review it, and delay its merging. This just wastes a bunch of time.

Do you have recommendations for how this might be included as a larger PR or grouped with other changes?

This could be grouped with anything that changes those particular test cases in a meaningful way.

I also have a template response:

Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

• Time spent on review
• Accidental introduction of bugs
• (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

For more information about refactoring changes and stylistic cleanup, see

• https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring
• https://github.com/bitcoin/bitcoin/blob/master/.github/PULL_REQUEST_TEMPLATE.md
• Code style PRs after v0.18 branch split #15465
• refactor: Continue moving application data from CNode to Peer #26621 (comment)

Generally, if the style is not mentioned nor enforced by the developer notes, we leave it up to the original author to pick whatever fits them best personally and then leave it that way until the line is touched for other reasons.

Let us know if you have any questions.

The time it's taken to get any feedback on a number of PR's I made 6 months ago has changed my focus away from core to other projects for now.

meaningful feedback for how I can contribute meaningful changes would be appreciated.

@yancyribbens It's not easy, and feedback is not only a technical process but also a social one. Anyway, feel free to ignore but some writings that may help:

• https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core
• https://jonatack.github.io/articles/how-to-contribute-pull-requests-to-bitcoin-core

@jonatack thanks for the response. I agree with you that feedback is not just a technical process but also a social one. You're links also help as well.

@achow101 @MarcoFalke thanks for taking the time to review.

s/you're/your