Reducing Pull Request Sizes

[0x4007]

Problem

It's horrible to see blocked up PRs accumulate tens of file changes with hundreds/thousands lines of code changed. It progressively gets more slow/tedious/difficult to review the PR and merge/close it

Context

It's really common to see merges from development to update the code as the PR ages, but then I see redundant file changes accumulate as part of the aged PR from the other already merged PRs.

Also when bounty hunters run a formatter with the incorrect settings it will introduce a large amount of unnecessary changes.

Proposal

We should design a limit for the bounty-bot to recognize where it urges the bounty hunter to redo their PR. For example, if 1000 lines total are changed, then the bounty-bot should leave a comment explaining the issue and urging the bounty hunter to fix it likely by reverting the damaging commit (this likely is either a full code formatting with the wrong settings, or a merge from development)

I think that we should start by only signaling/urging the bounty hunter to fix the problem with a warning message, but still allow the bounty hunter to proceed in case we need the flexibility.

[rndquu]

@pavlovcik

TL;DR
Bounty bot won't help, just keep issues small with clear specs and follow "1 PR 1 issue" appoach

I think you're just tired of reviewing PRs.

In order for a PR to be easy to review then:

1. Issue description should be clear
2. Issue should be small (we should divide big tasks into smaller issues)
3. Each PR should resolve exactly 1 issue

This is the latest huge PR ubiquity/ubiquity-dollar#417

This is the issue description ubiquity/ubiquity-dollar#406

The issue could be split into 4 tasks:

1. Create deploy scripts for local anvil instance
2. Update deploy scripts for development and production environments
3. Update UI to use contracts from local anvil instance
4. Devops: update UI to use contracts from development or production environments

The PR ubiquity/ubiquity-dollar#417 also solves other issues:

1. Added slither workflow https://github.com/ubiquity/ubiquity-dollar/pull/417/files#diff-073755564356b30d97f5d5fd4e8dc4fc398cf0d25a45d96195f76794575ff1cfR1
2. Solidity version updates https://github.com/ubiquity/ubiquity-dollar/pull/417/files#diff-3d7280c10b4ae1832642b615bc01a255103139b78154ec1909b336b90e5fe7faR2
3. Contract updates https://github.com/ubiquity/ubiquity-dollar/pull/417/files#diff-3d7280c10b4ae1832642b615bc01a255103139b78154ec1909b336b90e5fe7faR22
4. Test updates https://github.com/ubiquity/ubiquity-dollar/pull/417/files#diff-3ceb14e51fe3611bca81d0b6f63fe5ae04595e1628d52190ddf16a388d83f86aR67

But the issue's scope is to only create a deploy script

Also when bounty hunters run a formatter with the incorrect settings it will introduce a large amount of unnecessary changes

Yes, I remember, I just need more time for research of how to better format code so that it could be readable and not annoying to review

[0x4007]

This is the latest huge PR ubiquity/ubiquity-dollar#417

This is the issue description ubiquity/ubiquity-dollar#406

The issue could be split into 4 tasks:

1. Create deploy scripts for local anvil instance
2. Update deploy scripts for development and production environments
3. Update UI to use contracts from local anvil instance
4. Devops: update UI to use contracts from development or production environments

The PR ubiquity/ubiquity-dollar#417 also solves other issues:

1. Added slither workflow https://github.com/ubiquity/ubiquity-dollar/pull/417/files#diff-073755564356b30d97f5d5fd4e8dc4fc398cf0d25a45d96195f76794575ff1cfR1
2. Solidity version updates https://github.com/ubiquity/ubiquity-dollar/pull/417/files#diff-3d7280c10b4ae1832642b615bc01a255103139b78154ec1909b336b90e5fe7faR2
3. Contract updates https://github.com/ubiquity/ubiquity-dollar/pull/417/files#diff-3d7280c10b4ae1832642b615bc01a255103139b78154ec1909b336b90e5fe7faR22
4. Test updates https://github.com/ubiquity/ubiquity-dollar/pull/417/files#diff-3ceb14e51fe3611bca81d0b6f63fe5ae04595e1628d52190ddf16a388d83f86aR67

But the issue's scope is to only create a deploy script

This is because they merged from development in order to update the out-of-date code. From what I understand, they should rebase instead of merge OR handle the merge conflicts on a separate branch. The issue I want to avoid is to make sure that the PRs don't include redundant code changes for me to check from the other PRs. But if a separate PR is created specifically to solve merge conflicts then it could be reviewed in a cursory manner with the expectation that all of those changes are redundant. Not a perfect solution but will save reviewers a tremendous amount of time.

Proof of them updating the old PR is here, just look for merges:

So again just to clarify, the reason why this PR may appear to be "solving other issues" is because its just taking the work from other PRs which already solved the issues

Also when bounty hunters run a formatter with the incorrect settings it will introduce a large amount of unnecessary changes

Yes, I remember, I just need more time for research of how to better format code so that it could be readable and not annoying to review

We could actually just make a CI test for pretter --check or whatever. I think that will easily solve this issue.

[rndquu]

This PR ubiquity/ubiquity-dollar#417

There is a feat/406-protocol-deploy-script branch which is supposed to have only the deploy scripts. There are also other branches merged into feat/406-protocol-deploy-script, just check the commit history:

Merge branch 'Issue-284' of into integrate-slither
Merge branch 'integrate-slither'
Merge pull request #424 from ubiquity/pr-302-follow-up

So some of the already closed issues were parts of that branches. The source feat/406-protocol-deploy-script branch is different from the most common commit ancestor between the development and feat/406-protocol-deploy-script branches so you have to review changes again. I can't think of a silver bullet like some kind of CI prettier action or something else.

[0x4007]

The best solution that we have currently is to just encourage minimal PRs with angry comments from the UbiquiBot. PRs shouldn't merge commit to "update" from development, but instead (if they insist on updating) they should rebase. Rebasing should not include the redundant changes in their PR.