I was (almost) just that guy for one PR. Removed something like 20% or more of the codebase by leveraging the libraries and external tools we already had in use better, but it meant almost every single thing we were doing had to use the library function instead of the one we wrote. But assuming you have good regression tests and linters, so you know the code works and it's not terrible, the review should be more about overall high level quality instead of poring over every character to check correctness. It was still a pain to review, though

▲

jt2190 6 hours ago | parent | next [-]

You’re not an example of what we’re taking about here. Congratulations!

A better example would be if you’d changed the behavior of the library as you did this work, and the library changes introduced hard-to-detect bugs across the application.

	▲	wg0 5 hours ago \| parent [-]
		Yes exactly. the GP isn't what we are talking about it and huge PR isn't what we are talking about either. PR can be huge that's OK. For example, codebases that moved from Python 2 to Python 3 would have had huge PRs but the cognitive load was well understood.

▲

triceratops 6 hours ago | parent | prev | next [-]

Admirable effort. But why did you have to do it in one PR?

▲

stronglikedan 6 hours ago | parent | next [-]

> almost every single thing we were doing had to use the library function instead of the one we wrote

▲

triceratops 5 hours ago | parent [-]

Why not proceed file-by-file or directory by directory?

	▲	Neywiny 26 minutes ago \| parent [-]
		It was all in one giant multi thousand line file for one language and another for another. Which is also kinda an issue

▲

Neywiny 6 hours ago | parent | prev [-]

As per the other person's comment, yeah basically I could have broken it up but it would've been an arbitrary demarcation. I just deleted our functions and fixed everything that yelled. Admittedly that could've been one and then leveraging the libraries better could've been another, but they would've been 2 PRs that changed almost every line. So done as one to mitigate review time.

▲

nightpool 5 hours ago | parent | next [-]

Arbitrary demarcations can still be valuable! Just because something is arbitrary doesn't mean that it's not helpful. Working in chunks will let you take more time to review each callsite individually, and increase your confidence in the changes

In the future, I would definitely encourage you to explore a more iterative solution—fix the first 50 occurrences first, or maybe all the occurrences of a handful of functions. For example, if you have utility functions A, B, C and D, maybe fix functions A and B first, and then C and D second.

Ultimately, at the end of the day it's going to depend on how much code you're touching. If you're only touching 100 library calls, then it's probably easy to do them in one PR. But if you're updating 1000 library calls, you'll need to take a more iterative approach. Building those skills now will serve you well in the future when working on bigger codebases and harder refactors.

	▲	Neywiny 4 hours ago \| parent [-]
		Well another problem is that there was a developer also working on those functions at the same time. So just like the recent post on a 25 million LoC reformat done in a weekend, it seemed better to do it in one fell swoop. If it's good enough at 25 million, I'm sure it's good enough at a few thousand

▲

Uvix 5 hours ago | parent | prev [-]

Why not leave your functions but have them invoke the libraries instead?

▲

Neywiny 5 hours ago | parent | next [-]

They weren't drop in replacements. They were actually easier. Made up example:

> setup_terminal(); enable_input(); while(...) inp = read_character(); .....

> readline()

So yes I could've stubbed out the other stuff and replaced just one, but that's just adding tech debt

	▲	Uvix an hour ago \| parent [-]
		That makes sense. Thanks for explaining!

▲

spockz 5 hours ago | parent | prev [-]

It depends a bit. But it would now mean that there are multiple ways of doing the same. Call your internal function or call the library directly. You need to put up some linting around it that people only use your function or the library function.

Otherwise you may get that you have your function, you think everywhere is using it, you make it fix a bug. And poof, you introduced bugs at the other call sites.

▲

varispeed 3 hours ago | parent | prev [-]

> Removed something like 20% or more of the codebase by leveraging the libraries and external tools we already had in use better

Aka increasing the attack surface and maintenance burden.