[bbbobbb]

I know this is not point of the article but:

> The PR was bigger than what I felt I could sensibly review and, in honesty, my desire to go through the hours of work I could tell this would take for a project I no longer used was not stellar.

The PR: https://github.com/django-money/django-money/pull/2/files?di...

Do others share this sentiment?

This doesn't look like a particularly big PR to me, judging solely by the amount of code changed and the nature of the changes at first glance.

Are most of your PRs at work tiny, couple lines of code at most? Am I sloppy for not even consider reviewing this for "hours"? Are all code bases I have worked on sloppy because features often require changing more code than this?

[jakewins]

When I wrote this, almost all my engineering experience was in OSS database development. That environment has several forces pushing towards very detailed reviews and clean commit histories, like others have hinted at in the thread here.

The PR review is in public and heavily scrutinized by paying customers and passionate community members. APIs cannot be broken, and even with automated tooling it's very easy to accidentally introduce a change that breaks tens of thousands of deployments. And the code itself is really sensitive. If a bug gets in and released, it can be several days of grind to get a patch out, and after that many months of new tickets for the bug from customers that won't move to the latest patches.

Now I work somewhere where the code I write runs in-house. If a bug sneaks in, it's usually a 5-minute redeploy to resolve and the cost is borne primarily by my own team.

So I think the answer to your question is: it really depends on the environment you're writing code in. In some setups the cost of introducing mistakes is very high, so it makes sense to pay a lot at the review stage; in others the correct balance is less strict review and fast fixes/rollbacks instead.

[bbbobbb]

That makes sense, thanks for the clarification.

[akx]

The PR isn't very large, diff-wise, but IMO it isn't well made, which makes it hard to review.

Half of the commits are merges from other fork branches into the contributor's master, and the PR name and description doesn't mirror that in the least.

Then (eyeballing) 90% of the diff is whitespace changes, which would be fine in its own PR ("Formatting changes") because it's easy to eyeball that it's just that, but when you mix it with other changes, it's hard again.

[seedie]

FWIW, whitespace changes can easily be filtered out in Github's PR view.

[robertlagrant]

It's a mistake to filter out whitespace changes on Python diffs. These days it's best use `Black --check` or similar in CI to make sure they've matched your whitespace settings, to minimise these changes.

[akx]

Sure. The link above is in "hide whitespace" mode (`?w=1`), and there are still visible whitespace changes (new or removed whitespace lines).

[dingledork69]

Like 3 of them. Is that problematic enough that you cannot review the rest of the code? My eye just slides over them.

[DarkPlayer]

I am working on a GitHub pull request viewer that displays changes using a semantic diff, and therefore has some more advanced whitespace handling behavior than just ignoring leading or trailing whitespace. I tried it with this PR:

https://app.semanticdiff.com/django-money/django-money/pull/...

It doesn't make a huge difference, but it filters out changes like the added line break in "if value: value = str(value)" nicely. I haven't announced the project yet, but maybe someone will find it useful :-)

[mindB]

I don't think this was the case in 2016 though.

[wes-k]

They added it to the UI in 2018 but you could do it via URL since 2011.

> A diff view with reduced white space has been available since 2011 by adding ?w=1 to the URL.

https://github.blog/2018-05-01-ignore-white-space-in-code-re...

[seba_dos1]

Also, Git had it since forever, and since GitHub's diff view isn't particularly convenient for browsing multi-commit PRs you usually review the changes using Git anyway.

That said, I'd ask the contributor to tidy up the branch first. It's kinda disrespecting to ask others to review branches in such state.

[eyelidlessness]

After a brief scan I’d call the full change reviewable enough I could do it in a sitting. Most of it looks reviewable on my phone. But seeing >30 commits, I’d pause. Partly because I’ve become a lot more sensitive to the impact of commit history itself, partly because the quick scan of such a small change set doesn’t seem to line up with so many commits, but mostly because it implies much more context exists than the attention I’d pay if it came pre-squashed.

That kind of implication stops me in my tracks to learn more. I’ve spent literal days tracking down the meaning of single line code changes through multiple dozens of commits, sometimes across repo boundaries (ahem the original author’s suggestion of deprecating in favor of a fork comes to mind).

The size of this particular PR only becomes a factor when any one of those numerous commits can become that rabbit hole. How many humans’ days are going to be spent tracing history through this particular merge? For how many different reasons? I didn’t even look at the changes midway, but how many nuances are buried in there and lost unless this weird bundle of changes is preserved?

[hoten]

I bet you're other thinking in this case. In general, the expectation on GitHub is that PR commit history doesn't matter, and owners should simply squash on acceptance. I think most contributors don't event consider that all their commits are visible or would be of interest and only think about the final product.

It's certainly simpler for the contributor to do the squashing, but when GitHub makes it so simple in practice it doesn't matter.

[Arch-TK]

People wonder why Linus Torvalds dislikes github.

Imagine writing a highly performant and featureful relational database and successfully using it with large projects for a while without the database itself becoming particularly popular and then having a company come along and popilarise your database by telling lots of people about how good it is as a flat key value store.

Then people are really confused and annoyed as to why their key value store has this complicated and confusing relational database attached to it so they write lots of guides skimming over the details to help people get better at using the database to just store keys and values in one table.

If I was Linus I would be pretty pissed too.

[josephg]

That story strikes home for me. I worked at a company years ago where I designed and implemented a custom event queue like architecture using lua scripts in redis. The data would eventually end up in another database, but it would start its journey by being sent to redis. Once it was in redis the application servers considered it to be "committed".

Of course, sometimes bugs showed up in our system. One of the engineering leads would often say "oh lets just clear the redis cache". Every time I told him no, and once again slowly explained how we weren't using redis as a cache, and how deleting everything in redis would delete user data and be a terrible idea. He would have this far away look in his eye while nodding along and pretending he understood. I guess in his mind he was just thinking - why on earth would it be unsafe to clear the "redis cache"?

Months later I went on holidays. They ran into some bug. He reacted by wiping everything in redis. And, predictably, all hell broke loose. User data rollbacks happened, which caused cascading failures in the UI (which assumed that rollbacks would never happen). The team ended up reconstructing some lost data from some JSON which accidentally ended up in web request logs. Users had downtime as the whole app broke. It was a disaster.

When I got back to the office, I was hit with some strange combination of "why weren't you here" and "why didn't you tell us". Ugh. I still think about it sometimes. I have no idea how I could have handled that better. But I can tell you one thing for sure: I lost a lot of respect for that engineer.

[zarzavat]

As far as I understand, Linus doesn’t write code. He reviews code other people have written, and even other deputies review the code before it gets to him.

Additionally, many people are paid to work on his project by other companies. Linus doesn’t pay them, yet he’s their boss.

All of this is to say that Linus is very insulated from externalities. He can insist on his platonic ideal of a commit and SCM, if it makes his life easier. He’s like the editor at a publishing house, rejecting countless manuscripts yet never writing a word himself. And that’s fine.

However, most people do not use an SCM like Linus does. If you’re maintaining an open source project on GitHub you’re probably working for free, as are the people submitting PRs. The more difficult you make their lives, the fewer people will be willing to submit PRs and the more work you’ll have to do eventually.

[Arch-TK]

It's really irrelevant what Linus does now or what your personal opinion of version control is. The point is, git does certain things well, and a large portion of its users don't seem to need or want it. Not it might be because those users don't understand what they're missing out or whatever, but git was written by Linus from endless experience dealing with both contributing and maintaining code. It's simply wrong to suggest that Linux has no idea what he's talking about from a contributor perspective. Moreover, there's still plenty of people, who actually understand git, who are happy with the workflow that it was designed around. Yes it's a complex tool, but it's not as nonsensical or confusing as the people insisting on not using it to its full extent seem to always claim it is.

[akx]

I disagree pretty hard with this – for instance I've recently needed to dig into the code for the Gradio library, and when PRs are like https://github.com/gradio-app/gradio/pull/3300 (and the merge commit's message is what it is) it's hard to understand why some decisions have been made when doing `git annotate` later on.

[dboreham]

I don't think you're in disagreement. Parent is saying that it is customary nowadays to have crap commit history. You're saying that crap commit history is problematic from a s/w engineering perspective. Both are true.

[charrondev]

I’ve worked on a project with devs that write crap commit messages. It’s far easier to just commit to squash merging and requiring good PR titles and descriptions (which become the commit message following the squash).

Especially when you have multiple people working on a shared branch rebasing can be quite painful. The most common example of this is when sharing a branch with a QA.

[hardware2win]

I disagree, commits are mehh

Pull requests, patch notes, documentation and comments should be source of truth

Git is not project management tool, it just manages my letters history.

[oftenwrong]

Curious use of `annotate` instead of `blame`. Former user of cvs, bk, hg, bzr, fossil, etc?

[seabass-labrax]

My favourite tool for this is the blame command in Magit (the Git client for Emacs). You can cycle between styles; one shows the commit message as a kind of header in-line with the code for instance. Another just shows a faint rule between lines that were changed in different commits. Then, one can press return to show the specific commit that the line was changed in. Well worth a try if you haven't already!

[akx]

No-blame culture :grin:

(Also, JetBrains tools use "Annotate".)

[GrinningFool]

> In general, the expectation on GitHub is that PR commit history doesn't matter, and owners should simply squash on acceptance.

This varies greatly from project to project, and is by no means a general expectation.

[sodapopcan]

One sitting is a lot for a project you've forgotten about, lost context on, and are no longer excited by, especially assuming you have a lot of other stuff going on in your life. And then ya, everything else you said. And there are no tests.

[bombcar]

This is it entirely - imagine suddenly being asked to clean and organize a house you haven’t lived in for ten years. It may take you a day just to get back up to speed on the code.

(Interestingly enough Knuth praised literate programming for TeX as the reason he could get back in and fix bugs after an almost ten year hiatus - where parts of the code he had not looked at in 40 years.)

[seabass-labrax]

Some professional cleaners do indeed do their job on properties they might never have seen before; maybe the programming equivalent is the external consultant who is expected to quickly identify bugs in unfamiliar codebases.

[phkahler]

>> I’ve become a lot more sensitive to the impact of commit history itself

This was something @whitequark taught me. Having a clean commit history is very important when looking back, and we we look back more often than most people thunk. Self contained commits with good messages is important. I'm still not great at concise descriptions but trying.

[whstl]

+1 to this. I let my team pick one: very small PRs that get squashed when merging, or bigger PRs with well-separate commits and linear history that get merged normally.

Having a bunch of “fix” and merge commits in the main branch history is terrible.

[rtpg]

So with libraries that are consumed by third parties, it's rarely about the number of lines of code. Since you don't really have a definitive list of all usages of your code, things like the changes to `_money_from_dict` making things nullable mean you have to consider all ways in which that might blow up.

And like "the public API is the public API, the private API everything goes" is easy to say, but it's so easy to just break other projects with these kinds of changes. This makes it very hard to move forward with certain kinds of "bug fixes" in projects.

That being said, it's not that that PR is "hard", but it's hard to say "merging this is A-OK" instantly. Hours? I dunno, but I would definitely add some tests and try to figure out how to write code that breaks with those changes.

If I were managing that project at the time, and had motivation, I'd definitely do a lot of cherrypicking, get all the "obviously won't break anything" changes merged in, to leave the problematic bits in for a focused review. Again, this all might be under an hour, but sometimes you look at a thing and are like "I don't really want to deal with this, I have my own life to deal with."

At a higher level, the biggest problem with these kinds of libraries is having the single person who can merge things in, who can hit the "release" button. There's a lot of projects that interact with Django that have heavy usage, and survive mainly thanks to random people making forks and adding patches that properly implement support for newer Django. At $OLD_JOB we used a fork of django-money (including a lot of patches to fix stuff like "USD could be ordered with JPY", pure bug generators). It was very easy to add patches because, well, we had our usage and our test suite and no external users. It's great, but it's also important to try and get patches upstreamed when possible (and we did for a lot of projects).

[SenHeng]

Echoing your experience, I too worked on a project that was stuck on angular.js 1.3 when 1.6 was about to be released. We took a look at some migration guides and didn't see that many changes we had to make, so we tried updating it.

Everything broke.

Investigating, I realised that one of my long departed predecessors forked angular-bootstrap and made a few small changes to it. The problem was that the that library was tied to angular.js 1.3. To update angular.js, we had to update the library. To update the library, we had to remove all the changes in which would break large parts of our UI. The project was already in maintenance mode by that time and we decided to just leave it as is. I spent the next month converting it from coffeescript to es6.

[rpep]

I had exactly the same thing last year with a previous long departed staff member having forked django-flex-fields into their personal GitHub and having made substantial changes. Porting to Django 3.2 then became a huge and costly project in itself as a result.

[trevyn]

>So with libraries that are consumed by third parties, it's rarely about the number of lines of code. Since you don't really have a definitive list of all usages of your code, things like the changes to `_money_from_dict` making things nullable mean you have to consider all ways in which that might blow up. And like "the public API is the public API, the private API everything goes" is easy to say, but it's so easy to just break other projects with these kinds of changes.

Strongly typed languages, a well-designed API, and senantic versioning can make this problem largely disappear.

[rtpg]

I like typing and am a big fan.

If you fix a bug, this can break existing code. This is a fact of life. Changing performance characteristics can generate downstream problems! You have to consider this stuff seriously.

Here the change introduced nullability. In another universe the function would already be nullable but the conditions in which a value becomes None changes. A spec can be changed, for the better, and still be bug generating if people just upgrade. That is not captured by most type systems, and there aren’t that many great production web apps running on Idris.

But ultimately the reality is that people have a lot of flexibility with Python projects in general. It’s great, and libraries that are aware of this, well… they write it in release notes. They also have open repositories to enable actual code diffs. It’s non-zero amounts of work but it’s there.

There is a theoretical universe in which a static language with well-designed libs provide good aesthetics to make developing certain software easy. Meanwhile even as a big functional programming lover I still reach for Python because I can get work done because the libraries are in fact well designed, and the code is easy to work with, and I can fix issues quickly. As a user it’s great, as a library maintainer I gotta apply some more care. Could be better but it's alright

[TeMPOraL]

Largely, but not quite all. E.g. there's only so much information about runtime behavior you can capture in types, and then almost no one captures even half of it in practice. Doing this right requires a lot of experience, plus good knowledge of the problem domain, and it tends to both bloat and ossify the code.

It's easier if you're doing version 2 of a well-tested, scope-limited library, and thus can afford to do some holistic design. But the trend in our industry is to develop iteratively, small releases done often, everything kept at 0.x.y perpetual beta, and on the off chance your project grows old enough to warrant 1.x.y version, it has so much evolutionary baggage that you'd have to rewrite it from scratch to get proper typing in (which, of course, is against the zeitgeist).

[tsimionescu]

How so? The API still breaks if it's strongly typed, the breakage is just easier to spot by downstream consumers.

And note that saying "release as a new major version" is not making the problem disappear, it is simply choosing not to solve the problem, but with a warning on top.

[compiler-guy]

That has not been my experience. Nor Hyrum Wright's (of Hyrum's Law fame [1]), nor Randal Munroe's [2].

Note how things like performance characteristics leak through strong-typing, well designed APIs, and semantic versioning, in spite of non-guarantees around such characteristics.

1. https://medium.com/se-101-software-engineering/what-is-the-h.... 2. https://xkcd.com/1172/

[ShroudedNight]

One of the stories shared with me while working on OpenJ9 was of a customer that used stack overflows as a scheduling mechanism, and at some point they were compelled to update their JVM to a version that incorporated tail-call optimization, which caused their Rube Goldberg scheduler to stop working (by spinning indefinitely).

I would however argue that the existence of a user relying on behaviour that has explicitly been reserved as subject to change must not preclude development from rendering improvements to products. At some point the consumer needs to be on the hook for relying on a positive externality that they do not have a right to.

[hluska]

According to the article, the author no longer used this project. It was no longer front of mind. There is a massive difference between reviewing a PR when you are actively involved in a project and reviewing one when the project is in your past.

In that case, it’s perfectly reasonable to spend a couple of hours getting back into code you wrote a long time ago. If anything, taking that time is a big win for overall project stability.

[theden]

It's not too bad, but in the context of reviewing for a project in your spare time, it can be draining. I totally understand why he didn't feel like doing it.

[theshrike79]

I've committed bigger just with a "minor changes" -message :D

But seriously: that code seems to be touching bits that really should have automated tests attached. If the tests pass, then I would feel more comfortable accepting the PR.

[bruce511]

Depends on the bits they don't do. I get contributions from time to time. Yet can look small , but I need to add tests, documentation, and so on. It can easily take an hour for just one method added, or whatever.

If the code is just fire and forget, then fine. If it's part of a bigger system with rigorous standards then 300 lines can take a day or more to "merge" in.

[MentallyRetired]

It's certainly not hours of work to review it... or maybe it is since it's financial? Either way, "it was in the script" as my wife and I say about corny movie moments. It made for a good article.

[robertlagrant]

Critical Drinker has a phrase for nonsensical things in movies: "X occurred... so the movie can happen".

[bjornasm]

>This doesn't look like a particularly big PR to me, judging solely by the amount of code changed and the nature of the changes at first glance.

Well the author specified that this was their subjective take on it.

[femiagbabiaka]

For this PR in particular, seems like a lot of it is formatting changes, so you’re right, may not have been a big deal in practice. But I wouldn’t take the statement so literally. For an open source project, every PR can contain unbounded toil for no pay.

[xeromal]

It isn't that big at all, but when you maintain something you really don't want to maintain, you're over anything but the tiniest changes such as updating the copy. Ask me how I know. lol

[paulddraper]

One issue that makes it more inconvenient is there's no CI set up for this.

Some typo could break everything.

[golergka]

That's literally 300 something lines. I'm buffled, 5k PRs aren't that rare at work.

[ZephyrBlu]

You genuinely review 5k LOC PRs?

If I was doing a proper review of a PR that big and making sure you understand how everything works that would easily take multiple days and generate 10s-100s of comments.

In reality I would flat out refuse to review it. Even 1k is very large without being broken down.

The only time I see PRs that big at work are cleanups (E.g. deleting whole directories), automated linting changes across the whole codebase and large structural refactors (E.g. changing directory structure).

[opportune]

Almost every time I’ve reviewed a 1k+ LOC PR, even if it’s from a really experienced and good engineer, it has introduced a bug. Obviously I’m not gonna say it at work, but changes that big are too hard to properly review and consider all side effects and gotchas

[tstrimple]

It's also more difficult to bisect to find the actual bug. Small commits have a lot of advantages.

[TeMPOraL]

What is being changed matters.

To use an analogy: if you wanted to reupholster my car seats, or spice up the radio panel, sure knock yourself out - I'll come check when it's done. But if you were to as much as think about tightening or loosening a single screw anywhere near the engine block, believe me I will be paying very close attention to what you're changing.

[robertlagrant]

That would be a reason to never accept the PR; not to auto-accept it.

[zuprau]

I’d hate to work there. I’d rather review small chunks and merge often than review 5k lines that could be built on a shaky foundation/idea

[golergka]

Best team I've ever worked with. 5k was on the larger side, sure, but technically difficult tasks often just can't be split into a series of smaller pull requests.

[tsimionescu]

I am always amazed that many people with significant experience are so resistant to this idea. For what it's worth, my experience matches yours entirely: many significant changes can't be meaningfully committed in small chunks, they only make sense as an all in one.

And even more so, I've often seen the opposite problem: people committing small chunks of a big feature that individually look good, but end up being a huge mess when the whole feature is available. I hate seeing PRs that add a field or method here and there (backwards compatible!) without actually using them for now, only to later find out that they've dispersed state for what should have been one operation over 5 different objects or something.

[ZephyrBlu]

What changes can't be committed in <5k LOC? That's a shit ton of code. If you can't break that down into smaller shippable chunks there's probably something wrong, or you're building something extraordinarily complex.

It's definitely overall quicker to ship like this, but there are tradeoffs. You are effectively working independently from the rest of your team, there is no context sharing and everything is delivered at once after a longer period of time.

[rprospero]

Here's a personal example from work:

We're performing atomistic simulations. The first edition of the code stored each atom on the heap and had a vector full of pointers to the individual atoms. Obviously this would obliterate the cache, so I crafted a PR to simply store all the atoms in a single vector. On its own, that was a one line change, but it was also a very fundamental change to the type system. Everything as simple as

    Atom* linker = atoms[index];
    linker->x += 1.57;

Suddenly had to be

    Atom& linker = atoms[index];
    linker.x += 1.57;

If I didn't make those corresponding changes, the code wouldn't type check and the build would fail. I think the final PR came out to about 17 kLOC.

[ShroudedNight]

I've experienced this changing the API for a primary memory allocator in a frequently updated code-base. Each location updated was perilous and it needed to be changed in bulk to avoid an endless war of attrition.

[o1y32]

We have a small feature that was put together in a rush. It "works", but only works correctly for some simple cases, otherwise there are bugs everywhere. There were very few tests. We must redo the while thing, update the UX and add tests. Tell me how we can achieve that without replacing all existing code and add new tests that cover every use case in the same change.

[whstl]

Upgrading versions of a framework with breaking changes is one. I did a 12k PR a few weeks ago that touched almost every file of the app.

It was an all or nothing thing, as almost every third-party dependency we used had to be swapped by something else.

At least we have amazing test coverage, so it was easy to find bugs.

But there wasn't much I could say other than "trust me".

[sh4rks]

I think if a review is very large, the owner of the code should do a code walkthrough for the whole team.

[charcircuit]

>many significant changes can't be meaningfully committed in small chunks

They almost always can. The exceptions are stuff like autogenerated code or updating a dependency.

[midasz]

We as a team sometimes decided to PR into a PR branch, not as meticulous as a PR to develop but there'd still be eyes on the code entering the branch. Especially useful when there are dependencies and/or different disciplines contributing to the feature.

[globular-toast]

As if you are reviewing 5k lines, though. It's either 90% whitespace changes that you completely skim over (probably missing the one bit that actually changed) or you just skim over it looking for a few patterns you don't like such as using a loop instead of Array.map or something.

[sh4rks]

When they say 5k lines, I'm assuming they mean 5k lines of meaningful code. 5k lines with whitespace changes, linting etc. would be impossible to meaningfully review

[smabie]

This is so true.

[plugin-baby]

> at work

Level of trust with colleagues will hopefully be higher! And individual ownership of and responsibility for the code probably lower.

[g8oz]

Which is the philosophy behind Gerrit as opposed to Github.

[bjornasm]

Well, I feel like "at work" is a keyword here.

[nateberkopec]

What language/stack?

[golergka]

Typescript, React, Monaco, Treesitter, etc.

[quickthrower2]

Life is gonna life

[edem]

This is not at all a huge PR. Sometimes I make thousand line changes (or way over that), although in most cases it is just me working on a project. Changing a couple files (like in this PR) should be OK.

[kunley]

Do others share the sentiment that your reply is quite, sad to say that, arrogant?

What are you exactly trying to achieve by comparing the guy's "I'm busy, this is long" with yours or anybody elses? Moreover, what on earth has his job to do with the topic?

Bad day...?