Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

> How would you go about reviewing a PR like this?

Depends on the context. Is this from:

1. A colleague in your workplace. You go "Hey ____, That's kind of a big PR, I am not sure I can review this in a reasonable time frame can you split it up to more manageable pieces? PS: Do we really need a DSL for this?"

2. A new contributor to your open source project. You go "Hey ____, Thanks for your interest in helping us develop X. Unfortunately we don't have the resources to go over such a large PR. If you are still interested in helping please consider taking a swing at one of our existing issues that can be found here."

3. A contributor you already know. You go "Hey I can't review this ___, its just too long. Can we break it up to smaller parts?"

Regardless of the situation be honest, and point out you just can't review that long a PR.



If it’s the first one I’d be going a step further back to see how the work was defined. More often than not I’d expect the PR comes from a ticket that is too broad in scope and could have been broken down with a bit of architectural thinking.

The problem being that once someone has put together a PR, it’s often too late to go back to the serious thinking step and you end up having to massage the solution into something workable.


Telling a new contributor no thank you is hard. Open source contributors are hard to come by, and so I’ve always dealt with PRs like this (albeit before AI days but from people who had never written a line of code before their PR) by leaving a message that it’s a huge PR so it’s going to take a while to review it and a request to make smaller PRs in the future. A couple of times I ended up leaving over a hundred review comments, but most times they were all fixed and the contributor stuck around with many better PRs later.


> Telling a new contributor no thank you is hard.

In life in general having the wherewithal to say no is a superpower. While I appreciate the concern about alienating newcomers, you don't start contributing to an existing project by adding 9k lines of the features you care about. I have not run any open source projects that accept external contributions, but my understanding in general is that you need to demonstrate that you will stick around before being trusted with just adding large features. All code is technical debt, you can't just take on every drive by pull request in hopes they will come back to fix it when it brakes a year down the line.


The vast majority of PRs are bad. They could even be described as “selfish” in the sense that the “contributor” is haphazardly making whatever change minimally fixes their exact use case without consideration for the project’s style, health, usability, or other users. This isn’t outright malicious or even deliberately inconsiderate, but it still has a negative effect.

Refusing such a PR (which, again, is most of them) is easy. But it is also time consuming if you don’t want to be rude. Everything you point out as inadequate is a chance for them to rebut or “fix” in a way which is again unsatisfactory, which only leads to more frustration and wasted time. The solution is to be specific about the project’s goals but vague about the code. Explain why you feel the change doesn‘t align with what you want for the project, but don’t critique specific lines.

There are, of course, exceptions. Even when I refuse a PR, if it’s clear it was from a novice with good intentions and making an effort to learn, I’ll still explain the issues at length so they can improve. If it’s someone who obviously used an LLM, didn’t understand anything about what they did and called it a day, I’ll still be polite in my rejection but I’ll also block them.

Ginger Bill (creator of Odin) talked about PRs on a podcast a while back and I found myself agreeing in full.

https://www.youtube.com/watch?v=0mbrLxAT_QI&t=3359s


Git is flexible enough that you can tell people to break up their PR. They don't have to redo all their work.

If you want to be really nice, you can even give them help in breaking up their PR.


Yeah exactly, the OP describes a completely new service built start to finish all in one merge request, where normally you'd start with a proposal and work from there.


You can even create the proposal retroactively from the PR, if you already have the PR.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: