Also, please, keep your PRs small and fixed in scope. This particular change snuck in on a large PR, to which it was added only fairly late on the review process.
One project I worked on some guy presented me with his masterpiece, a ridiculously large refactoring of the entire code base and clean-up of old code that was no longer used. He was really pissed off when I categorically rejected the request and asked him to break it up into manageable pieces.
Quite sad because obviously he put a lot of effort into it but he did not keep in mind that it's one thing to do this, it is an impossibility to actually review it.
When coding keep in mind the job of the people coming after you: review, QA, eventual long term maintainers and so on.
Anything else should be done incrementally, in small manageable commits.
You can take as much time as you need in order to give a thorough review and express your frustration so that it's clear next time he should strive for something more manageable, but catagorically rejecting a large code change is just silly and juvenile.
This is why I hate agile. It's conditioning people to believe the only thing you can do to a codebase is incrementally add features. Sometimes the most meaningful or beneficial work comes in the form of broader systemic changes to a codebase.
And it sounds like you are quick to rush to judgment.
You're essentially accusing me of doing exactly what you are doing. You could have instead asked more about the context within which this all happened (which was a pretty bad situation to begin with) and if I tried to work with the person submitting the pull request.
Instead, you make me out for lazy (which is something that I don't think anybody that knows even a little bit about me) and apathetic (which also doesn't fit the bill, rather the contrary).
You can take as much time as you need to do a review if you have that time. The middle of a house fire is not the right moment to push through your pet project, and it also doesn't mean that if I point blank refuse a pull like that that I had not thought it through.
One argument against gradual refractors is that they are often left half done when enthusiasm runs out. I’ve seem mloc sized code bases with multiple layers of half done refactors and migrations.
This was part of the problem for the Segwit2X code: inadequate testing. The original Bitcoin Core codebase is up to a very high standard, and that high standard seems to be the first thing to go anytime someone goes to fork it. This is part of the reason why the developers of Bitcoin Core have so much influence to set future direction of the currency within the Bitcoin community at large; there aren't that many highly-skilled cryptocurrency developers out there. It's unsafe to launch a fork without them.
I'm putting off a very sizable one at work (due to important functionality going in before FCS), and luckily, although it'll hit nearly every file, I can do them individually.
My concern, actually, is eyes glazing over on the n'th review that's vastly similar to the previous n-1.
Refactoring is a bottom-up design tool. You remove accidental complexity and uncover higher order organization in your code, occasionally uncovering or allowing new functionality that was intractable before.
A “large refactor” is a top-down change. Major surgery. You’ve used (maybe misappropriated) the tools of refactoring for a different purpose which is antagonistic to the school of thought that enumerated these tools.
I say this as someone who when I was starting out spent a holiday weekend rewriting hundreds of lines of repetitive code (a bad idiom) to move an O(n²) performance issue into a single function. I removed 500 lines of code in the process. At that point, and for years after, my most productive week.
Enter the Mikado method. With Mikado you start to do your large refactoring. You change A, which forces you to also change B. You change B, but now C has to change too. So you change C and, you guessed it, now D doesn’t work. Man this is getting out of control. D leads to E, and E leads to F.
Now you’re looking at making change F. You are worried, or rather, you should be. How are you going to verify all these changes are complete? How do you know here isn’t a G, H and I waiting for you after F?
Now the hardest thing for any developer to do (besides admitting they gave bad advice) needs to happen. You need to throw away code you just spent hours of time sweating over. Revert to master, and start over.
Just implement change F, by itself. Sure enough, F leads to G and H, but those three changes are still reasonably sized for a code review. Send it off and start working backward up the list.
The reason we do bottom up is that we uncover the difference between our intuition about the code and the situation on the ground. When you go to do E or D you might find you didn’t need to make the change quite as broadly as you thought. And you might be able to skip A and B entirely.
When I do Mikado, I find that only about 60-70% of the code I originally wrote survives the process. The changes are more contained, which means they impact fewer of your coworkers less often. They are easier to reason about, even self-explanatory. Of course you would do it this way, it just makes sense.
The interesting thing to me about Mikado is that some people independently discover the trick themselves. In fact these large refactors I had to do, I complained, reminded me of playing pick-up sticks. I can’t move this because of that. Can’t move that because of the other thing. I got overwhelmed by the size of a couple of these changes, and threw them out. Then started over either immediately or at a later date.
Because part of refactoring is realizing that certain problems can be fixed at any time you deem it necessary to do so, that means you don’t have to fix it just because you can. A lot of my code ends up prepped for a refactor I never do. Someone else does it for me, when they have a requirement that makes the change make sense. It’s my 90-10 compromise for the tension between YAGNI and broken windows. I don’t make the change, I just make the change easy.
But if safety does not matter, or its pure crap anyway. Then refactor away.
Proper review of core components that are used and proven is hard already, refactoring such components - don't do it if avoidable - there must be a very good reason. And convenience ain't one - you'll probably get some duplication of code and double maintainance during the transition period.
Making small PR is just a way to be safe and smart, one branch with individual gradual commits that could be used separately is also a way (if they could be reviewed separatley).
Maybe that would occur if you had to replace a central algorithm with a significantly different one (because the original one did not scale, for example). That is more of a rewrite than a refactor, and should be done as a separate product, developed incrementally, of course.
If 'refactoring' did only refer to simple, mechanical changes like renaming, I could see it, but you have just argued against that interpretation.
With lots of testing, yes, you can incrementally build a working system that you don't understand. But by myopically staring at diffs all day, you'll only get farther from the truth of how your system works.
The major refactors that I do, are because I've come to the realization that my data model was not correct. A realization, that often happens multiple times during a project. My utility functions are still useful, but I have to rewire everything. There is no meaningful way to do that incrementally. Why would you even try to do that incrementally? It's like, if you have a salamander and you want to turn it into a horse incrementally. Of course, most of the RNA is the same. But testing and code reviewing the steps between salamander and horse seems to me like a waste of time.
While "if I understand the steps which got me here, than I understand where I am" may not be guaranteed (especially if you don't know where you started from), it is almost always among the more effective ways to proceed (though it is not particularly helpful if you don't know where you are going.) It can help avoid getting lost, which is one of the reasons for doing development incrementally.
There is no way in which I can see that turning a salamander into a horse has anything to do with refactoring.
In static languages such as Java, renaming a variable is something your IDE can do for you with all the benefits of static analysis. There's almost no risk associated, and reviews can be a rubber-stamp.
/s