5252418df0
@tgstation/commit-access I'm proposing a new use for the Atomic tag that we currently virtually never use. We have countless pull requests over time, and plenty of which open now, that are enormous refactors over tens of files with thousands of additions. We are historically pretty slow to review and merge these, and it definitely scares a lot of maintainers off. I think part of the reason is that we do not like dead code being added, which is completely reasonable at our scale. However, I propose that, for refactors/purely code stuff, we ease up on this a lot, and encourage (not require) people to make smaller pull requests, even to the extent that it creates APIs we do not use yet. As an example, https://github.com/tgstation/tgstation/pull/71421 does a massive refactor to carp. It also does some balance changes, which I think we could agree could be split off if it was enough of a pain. However, there's a bunch of other stuff that could have been individual pull requests here with this new allowance. - The new basic AI behaviors - The regenerator component - Pet commands component These are things that: - Would not be used until the transition from simple to basic, but are easily reviewable on their own - Are easy to REMOVE if the OP does not follow up - Are easy to FINISH if the OP does not follow up (I suspect even, for instance, that there are parts of Wallening we could be merging right now, that's probably gonna be hundreds or thousands of files long...) Pros: - PRs are more often easily reviewable - PRs are quicker to merge, since we don't have conflicts from editing one of the 70 files they changed - Cleanups can be more easily finished by other people. I don't suspect this will be likely, but it's not easily possible today Cons: - We have to mark the PRs as atomic - Someone needs to look through every so often (I'm thinking like, once a month or something) to see if the code ended up being used, or if the committer still plans to use it - If the PR is adding a complex enough API that isn't modular, it might be very hard to remove. I suspect for PRs like this that we ask them to have an implementer before merging. NL voice would love your thoughts on this
2.2 KiB
Atomization (AKA, splitting up pull requests)
Maintainers are volunteers and have limited time to review pull requests. Large pull requests can be hard to review. You can help us help you by splitting your PR up into more manageable chunks.
Keep it on topic
A pull request for changing the color of airlocks should not also change the damage for guns.
In general, keep balance PRs separate from fix/refactor PRs, unless there is a reasonable explanation for having them in the same pull request. For example, if in your PR for changing airlock colors, you clean up the code to improve variable names in airlock code, that is completely fine to keep in the same pull request. Fixes are something we can merge if the code looks right. Balance changes/new features are something we can only merge once we evaluate design concerns, which is much slower.
Split it up if it's big
Sometimes a refactor might end up being a lot larger than you expect, and suddenly it is very hard for us to review, even though it's all on topic.
We encourage contributors to, when reasonable, split up huge refactors into several chunks, or to split them off from your features entirely. It is even okay to make a small pull request to add an easy to review API even if the code is unused, just be up front about the changes you are planning to make. These pull requests will be tagged with the "Atomic" label. Every so often, maintainers might look at past PRs with this label and remove the code if it is still unused, or if you have let us know you don't plan on finishing it (though in this case, please remove it yourself). When you should do this isn't written in stone; if adding a consumer of the API is just a matter of a few lines, then you should just do that. On the other hand, if adding this API requires touching a lot of existing code, and would be hard to remove if you don't finish, you may be asked to put it in the PR that adds the consumers.
This does not necessarily apply to features/balance changes. We don't want half of a feature implemented because it leaves you and us uncertain to if the followup pull requests are going to be merged. For example, do not add a new item to the game that goes completely unused until a separate pull request which decides where it's going to go.