A lot of modifications in pull requests

Question

I would like to know how manage a type of pull requests. One of my reviewers ask me for a lot and a lot of changes (3 or 4 days doing changes), all related with possible improvements in the code that I thought it is too much and it ends up in a "perfect" and "smart" code. It is difficult to discuss with a person who try to abstract a lot and type the perfect code... but I have a feeling that sometimes, simple is better and use a lot of concepts for small modules is "overkill" an idea and we should wait a little for the refactorization.. It means, I always thought it doesn't exist one way to do the things but I can be wrong and this is the "normal" way to the things and the refactorization always is better.

One example can be:

• Create multi inheritance with multiple layers for a better abstraction. In this case, it can be a perfect the abstraction but for 3 classes that we don't know how they will scale it, we could wait a little to verify better if we need these abstractions.

Answer 1

Do you need to do everything that your reviewer asks of you? Usually, you don't. A good code review process has some back-and-forth. A reviewer might suggest a change, the developer might explain why that is or is not a good idea. If the reviewer's suggestions really improve the code, you should consider them. If you don't understand why the reviewer's suggestions might improve your code, ask them.

If you are concerned that they are asking for too much in one pull request, talk to them. Ask them if it's OK to create a separate issue for specific, non-essential refactoring improvements. Then you can complete the PR as it is, and move the improvements to a separate task.

Answer 2

If such major issues arise at the code review stage, it is too late. It shows that your development process has problems. In a normal process first the main design decisions should be defined and discussed within a team. Then there is not much space for discussion.

Then I see 3 major cases.

1. If you followed the previously agreed solution design, then nobody can ask you for changes that deviate from that defined design.

2. If the reviewer is aware of design decisions, but finds then at the moment of review not more good, then this should be addressed separately, as a new change or new feature, but still there is no reason to require anything that is not defined in the design decisions.

3. If you have misunderstood some design decisions, then you should accept such review and rework your code.

From your question I suppose that there was no solution design defined for your feature. Then don't change anything and discuss it in the team or with an architect responsible for your application. In some cases it can make sense not to merge your code and to rework it, in other cases the changes requested by reviewer can be just rejected, in some cases in can make sense to merge your code and to implement the requested change in the next iterations.

Answer 3

The question is at this point: should this set of changes have been made? If you say the answer is “no” then there is trouble obviously. The answer “no” should have been given before the change was started. And the decision shouldn’t be part of the code review, but should be made outside.