Clarification on Pull Requests

[cschuchardt88]

We all have a different perspective on how Pull Requests should be done.

Can we get some clarity? So, we all are the same page.

• How big is big?
  • How should it be broken up?
    • With examples!
  • Timeframe of merging?
  • Should we wait for a merge to continue to next PR?
• How should new projects or applications be done?
• How should downstream modules be handled.
  • Timeframe of merging?
• What happens if broken up PRs will break github workflows?
  • How will merging happen of those PRs?
  • How should that PR be handled, that breaks changes?
• What are the categories of things that need to be broken up in different PRs?
• Why are we merging when, no one is following guidelines or code is not consistent with existing?
• Should there be no merging, if these rules are broken? example: I'll do in another PR.... Then it never gets done.

Solution

---

We need some guidelines and rules; having a timely review with merging, no more half a year for a merge; a good review and no more adding code to repo thats looks like someone copied and pasted it from stackoverflow or that breaking coding styles that we agreed on.

@shargon @Jim8y @vncoelho @superboyiii we all need to agree on something. I have no problem reviewing miles long PRs. So we need rules I understand that. Let's all agree on something here in this issue.

Just a note, even super small PR can take months for a merge or a review.

[roman-khimov]

How big is big?

>200-400 LOC. No exact definition, of course. 200 LOC can be a hell to review in some cases. 1 KLOC of stylistic changes are the opposite.

How should it be broken up?

Multi-PRs and multi-commits. I especially love proper patches commits.

With examples!

A set of organized commits:
https://github.com/nspcc-dev/neo-go/pull/3402/commits
Maybe also
https://github.com/nspcc-dev/neofs-node/pull/2814/commits

A set of PRs with a set of nice commits:
https://github.com/nspcc-dev/neofs-node/pull/2753/commits
https://github.com/nspcc-dev/neofs-node/pull/2801/commits
https://github.com/nspcc-dev/neofs-node/pull/2802/commits

Timeframe of merging?

When it's done.

Should we wait for a merge to continue to next PR?

Depends on how issues/PRs are related. In many cases it's not a blocker, you're touching different parts of the code in different PRs. If there is a clear dependency (like #3175 and #3178), it's up to you, either you want/need to show the next step or you can wait.

How should new projects or applications be done?

Too broad question.

How should downstream modules be handled.

Any breaking change to the core should be accompanied by update to downstream modules.

What happens if broken up PRs will break github workflows?

They shouldn't. Workflows should be green before the merge. Each individual chunk of work should be OK on its own. If we have a regression after the merge because of unexpected relation between different PRs we just fix it.

What are the categories of things that need to be broken up in different PRs?

Big changes.

example: I'll do in another PR.... Then it never gets done.

Create an issue, it won't be forgotten this way.

We need some guidelines and rules

No doubt about it.

no more half a year for a merge

Everyone hates it. But it's not something we can solve easily.

[cschuchardt88]

@roman-khimov

How should new projects or applications be done?

Too broad question.

For example #3103 it's getting pretty big, and will be big. However once ready for testing it wont have all the features.

[roman-khimov]

I'm absolutely sure it can be split into logical parts. Take your "features" list and try to think of it in terms of "one feature --- one PR".

[shargon]

I am absolutely agree with @roman-khimov, I will try to improve my commits, but one thing is clear, a big pr is hard to review, and something hard to review easily introduce new bugs