undefined | Better HN

0 pointsNeywiny10d ago0 comments

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.

0 comments

nightpool10d ago

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.

NeywinyOP10d ago

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

nightpool10d ago

Autoformatters are deterministic tools that are tested regularly, with extensive test suites and went through a very long process of production usage and review before the reformat:

    > We also built a tool to diff ripper trees across formatted files, accounting for things like rubyfmt converting single quotes to double quotes. Combined with our extensive test suite, we built confidence slowly and deliberately.

If an autoformatter is working right, it's only changing whitespace—not the actual code executed. Changing between two different implementations of the same function is very different from changing whitespace around.

They also didn't do the entire thing in one weekend—that was just the article title clickbait. they did it file by file, incrementally, over the course of months:

    > Rolling out a novel autoformatter to 25 million lines of code has two big risks: merge conflicts and correctness. A bug affecting just 0.01% of lines would still touch tens of thousands of files. To manage both, we built in a per-file opt-in so rubyfmt would only format files that explicitly asked for it. Following the Developer Productivity org’s typical pattern, we started with systems we owned and could observe closely, then expanded coverage gradually as our confidence grew.

^ See how they talk about the incremental changes it took? This is what mature refactors look like. And they were only changing whitespace!

Uvix10d ago

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

NeywinyOP10d ago

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

Uvix10d ago

That makes sense. Thanks for explaining!

spockz10d ago

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.

j / k navigate · click thread line to collapse

0 comments

nightpool10d ago

NeywinyOP10d ago

nightpool10d ago

Autoformatters are deterministic tools that are tested regularly, with extensive test suites and went through a very long process of production usage and review before the reformat:

    > We also built a tool to diff ripper trees across formatted files, accounting for things like rubyfmt converting single quotes to double quotes. Combined with our extensive test suite, we built confidence slowly and deliberately.

They also didn't do the entire thing in one weekend—that was just the article title clickbait. they did it file by file, incrementally, over the course of months:

    > Rolling out a novel autoformatter to 25 million lines of code has two big risks: merge conflicts and correctness. A bug affecting just 0.01% of lines would still touch tens of thousands of files. To manage both, we built in a per-file opt-in so rubyfmt would only format files that explicitly asked for it. Following the Developer Productivity org’s typical pattern, we started with systems we owned and could observe closely, then expanded coverage gradually as our confidence grew.

^ See how they talk about the incremental changes it took? This is what mature refactors look like. And they were only changing whitespace!

Uvix10d ago

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

NeywinyOP10d ago

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

Uvix10d ago

That makes sense. Thanks for explaining!

spockz10d ago

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.

j / k navigate · click thread line to collapse