(I'm sorry, this post is going to jump around quite a bit -- I don't have time to structure it properly).
I think there are several reasons that PRs sit in the queue for ages, but fundamentally they all come down to looking at the PR and being hit by that sinking feeling that "this is a mess and it's going to be difficult to deal with". This is a feeling that puts me off reviewing things and makes reviewing just one non-trivial PR feel like a pretty big effort. Hell, just
looking at the PR queue can be draining when you know that a) they're not in a state that you want to just merge immediately, b) the problems aren't just clear bugs that you can just throw back to the submitter with a quick two sentence description/explanation.
I can step down from project stuff, ignore the PR queue and just work on code. There's nobody that really wants to pick up all the stuff I do.
If you're fed up with the PR queue and want to stop dealing with it, then I won't try to convince you to keep at it. You've always been clear with me that Pioneer is for fun, and none of us have an obligation to work on things we don't want to, and I would like to say the same thing to you. Pioneer is for fun, and you do not have an obligation to deal with pull requests if that's not something you want to do, and that is true
regardless of any impact on how fast the project progresses.
I can stop writing code and just do project stuff. I've tried that before on a previous project. I burned out and it took a year before I wanted to touch a computer outside of work. I'm rather keen to avoid that.
I don't want you to burn out either, so unless you can see something seriously different about Pioneer then I would advise against this.
I can open git access to everyone (and here's some reading on a model that might work). Same danger.
I wouldn't try to block this if you think it's worth a shot, but I suspect it wouldn't work well for us. There are practical issues of level of confidence with git and level of programming experience that would put a lot of people off merging things, or could result in confusion or other problems.
robn wrote:I can lower my standards, mostly just merging anything that comes along. I worry that the lack of direction will kill is (and we do have some direction, believe it or not). I also worry that it'll add more mess to the codebase. That's going to feel too much like hard work being undone, and I won't be able to watch it for long.
For what it's worth, I think this is the most practical option of the ones you've listed.
I said above that PRs sit around for ages because they feel hard to deal with. This is a psychological problem, not a technical one. We need to find ways to make processing the PR queue easier, not in terms of technical effort required, but in terms of the mental and emotional energy required.
Perhaps it will help to talk about what types of problems PRs can have, and make some guidelines about how to deal with them.
- Bugs, logical errors, mathematical errors, typos (any simple, isolated code error). These are easy. If we (or someone else) finds a bug during testing or by looking at the code, either fix it or tell the submitter to fix it. It's easy because everyone agrees that bugs need to be fixed.
- Uncertainty about some code logic or behaviour ("Is this maths right?" "Did the submitter intend this weird behaviour?"). These are easy: Ask the submitter (obviously).
- Code style errors (braces, whitespace, naming conventions, etc). Same as bugs: fix it, or tell the submitter fix it. Not everyone agrees about the importance of code style, or perhaps even what style we should be using (particularly since the existing codebase is not totally consistent), but style problems are easy enough to fix that if someone wants to argue about them instead of just fixing it and moving on, then that person probably isn't worth dealing with. If there is some aspect of style not covered by the code style guide on the wiki, amend the guide and tell the submitter to fix their style. Yes, style is subjective, and it's not the submitter's fault that they didn't use the "correct" style, since it may not have been clear what was the "correct" style when they made the PR. I don't care -- the time and energy to discuss it costs more than just fixing the code.
- Messy git history. Fix during merge. I've given up on asking anyone to clean up their own git history -- a lot of our contributors just don't have enough confidence with git to do it right anyway, and the ones that do tend not to submit messy histories in the first place. Therefore, it's not worth anyone's energy discussing it. Whoever is processing the PR can rebase, split and squash commits as necessary.
- Game design uncertainty ("Do we want this feature?", "Do we want it to behave this way?"). Close PR, tell submitter to start a thread on the dev forum to discuss it. Tell submitter that it is their responsibility to make sure that discussion reaches a conclusion -- if the discussion actually does reach a conclusion (I'll believe that when I see it!) and the conclusion is that we do want the feature, then the the PR can be reopened or the submitter can resubmit if there were design changes needed.
- Code structure problems ("We want the feature, but not implemented like this"). I think this is by far the hardest one (a lot of the above we already deal with as I've described). Structural problems can take a significant effort to fix, they make future code maintenance harder, they're problematic all round. This is the one that that sucks PRs into a timeless void, and this is the one where robn and I need to lower our standards and be more willing to merge things that we know will need to be ripped out or totally rewritten later.
I'm going to ramble about the last one for a bit, because it's the hardest.
My instinct is to try to filter out things of questionable quality or desirability at the review stage, to try to keep master on a steady path of improvement and avoid adding code that I believe we will have to totally rewrite. Even more strongly my gut tells me that it's bad to add Lua API or content format features if there's a serious chance of that interface having to be removed or significantly changed later. But, all the evidence I can see suggests that
that just isn't working -- Pioneer is improving, but code quality is certainly not following a monotonic path upwards.
My experience has been that trying to fix structural problems by pointing them out to submitters is very hit-and-miss (emphasis on "miss"). It's also my experience that discussions in PR comments about problems of how things are implemented are irritating and energy-sapping for me and probably for everyone concerned. It may be defeatist of me, but the only solution I can see is to minimise that type of discussion. If there's a structural problem, robn or I should fix it when we merge, or if that will take too much time, then we should add some fixme notes to the code and merge it despite its flaws.
The only options I see are: throw it back to the submitter telling them the structure isn't right (which doesn't work), close it (which is just throwing it back more forcefully, and I don't see any reason it would work any better), or merge it and accept that it means more stuff that needs to be rewritten in the future.
This isn't a
good solution. It's not a solution I like. I don't think it'll be easy (merging bad code feels bad). But I think the alternatives might be worse.