search

neo-project / neo

NEO Smart Economy
MIT License
3.46k stars 1.03k forks source link

Clarification on Pull Requests #3208

Closed cschuchardt88 closed 1 month ago

cschuchardt88 commented 5 months ago

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.

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 commented 5 months ago

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 https://github.com/neo-project/neo/pull/3175 and https://github.com/neo-project/neo/pull/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 commented 5 months ago

@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 commented 5 months ago

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 commented 4 months ago

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