I agree on including combat. My remaining "AI won't build cities" PR also adds a couple more AI players (three total; we could bump it beyond that but three is much better than one), as well.
I also agree that we need a policy of "if not reviewed within X time, it can be merged." Caro-Kann (Lucien) proposed 5 days. I think two weeks is a little bit too long as if there is a period of time where one person is doing a lot, that can be a lot of PRs being introduced and building off of each other. I saw that in February when there was a time when I was creating about one per day. I tried to keep them seperately mergable in some cases (in others that was not feasible), but even so there wound up being conflicts between them. Perhaps 1 week is a good balance? I'll also note that this makes 3 of the top 5 contributors who have said we should have this (regardless of the precise time); I don't think Puppeteer has commented, and WildWeazel advocated for the review process, but I don't think has specifically commented on this proposal.
One of the teams I worked on had a process of two tiers of reviews, for just the "it doesn't feel appropriate to approve it since I haven't done a real review, but I have no objections" category. They had a +2 approval for "I've done a real review", and a +1 approval for "I've looked at it and have no objections, but don't feel comfortable saying it's definitely good," which was commonly used when reviewing areas of the code that you didn't have a great deal of expertise in - you didn't see anything flawed, but weren't an expert in the area, or perhaps you took a more cursory glance at it due to time constraints (the team, which was much larger than ours, and a commercial team where most people were there most days, required two +2 and two +1 approvals, so that could still help it move through the process).
I kind of did the same thing with the "static variable in Util", noting that it looked good to me and I'd tested it on Windows (but not elsewhere). I guess I'd say it's better to leave a comment saying what you mentioned here, since it still increases the confidence somewhat, but not necessarily giving the full review-approval if it doesn't seem appropriate. In the "static variable in util" case, my best guess is it hasn't been merged since Puppeteer has been working on other things? In a couple other cases where the contributor of a PR had gone idle and the PR's work was something I neeeded to build on, I eventually merged it myself, but since that one hasn't blocked me I haven't done so. But realistically, we probably need to say, "if a PR has been approved for X days, anyone can merge it."
The same +2/+1 approval team was one that tended to have a lot of large PRs ("large" in this case being 1000+ lines). I spent a lot of time thinking about PRs on that team, and concluded the point at which my motivation starts to noticeably wane is around 300 lines, and that there's at least a bit of a quadratic element to it, rather than purely linear. I was reminded of it recently while reading an article on Utah's BAC (blood alcohol content) limit of 0.05. They mentioned that research shows the effect of impairment on a driver is not linear, but again somewhat quadratic, but until the 0.04-0.05 range it's low enough to not be really noticeable. But someone who has a BAC of 0.16 is functionally much more impaired - noticeably more than double - than someone just at the typical state's 0.08 limit. It's similar with PRs for me. Three 300-line PRs is less work than one 900-line PR. Even considering that breaking it up into smaller chunks might result in more net lines changed (maybe that 900-line PR becomes four 300-line PRs), I'd rather have the smaller ones.
I've tried to follow that with the Settler AI merges; it's definitely not complete, but each one makes it a little better. When there are larger merges, having notes of "there are basically three entry points: SomeClass.cs, SomeOtherClass.cs, and ThirdClass.cs", or some other indication as to the high level structure would help. Sometimes I'll break large ones down by commits, but that isn't as feasible when there are dozens of commits. And it winds up being the case where, if I'm getting paid to review those large changes, they'll get done (but more slowly than if they had been several smaller ones), but if I'm not getting paid for it, it's hard to resist making improvements to my Civ3 scenario editor, or watching an Arnold movie, instead. (Although today's choice, of enjoying the first warm weather day of the year, would have been selected no matter how appealing any potential PRs were)
Looks like the Threaded arch one is only 1349 lines when whitespace diffs are hidden, though...
I also agree that we need a policy of "if not reviewed within X time, it can be merged." Caro-Kann (Lucien) proposed 5 days. I think two weeks is a little bit too long as if there is a period of time where one person is doing a lot, that can be a lot of PRs being introduced and building off of each other. I saw that in February when there was a time when I was creating about one per day. I tried to keep them seperately mergable in some cases (in others that was not feasible), but even so there wound up being conflicts between them. Perhaps 1 week is a good balance? I'll also note that this makes 3 of the top 5 contributors who have said we should have this (regardless of the precise time); I don't think Puppeteer has commented, and WildWeazel advocated for the review process, but I don't think has specifically commented on this proposal.
One of the teams I worked on had a process of two tiers of reviews, for just the "it doesn't feel appropriate to approve it since I haven't done a real review, but I have no objections" category. They had a +2 approval for "I've done a real review", and a +1 approval for "I've looked at it and have no objections, but don't feel comfortable saying it's definitely good," which was commonly used when reviewing areas of the code that you didn't have a great deal of expertise in - you didn't see anything flawed, but weren't an expert in the area, or perhaps you took a more cursory glance at it due to time constraints (the team, which was much larger than ours, and a commercial team where most people were there most days, required two +2 and two +1 approvals, so that could still help it move through the process).
I kind of did the same thing with the "static variable in Util", noting that it looked good to me and I'd tested it on Windows (but not elsewhere). I guess I'd say it's better to leave a comment saying what you mentioned here, since it still increases the confidence somewhat, but not necessarily giving the full review-approval if it doesn't seem appropriate. In the "static variable in util" case, my best guess is it hasn't been merged since Puppeteer has been working on other things? In a couple other cases where the contributor of a PR had gone idle and the PR's work was something I neeeded to build on, I eventually merged it myself, but since that one hasn't blocked me I haven't done so. But realistically, we probably need to say, "if a PR has been approved for X days, anyone can merge it."
The same +2/+1 approval team was one that tended to have a lot of large PRs ("large" in this case being 1000+ lines). I spent a lot of time thinking about PRs on that team, and concluded the point at which my motivation starts to noticeably wane is around 300 lines, and that there's at least a bit of a quadratic element to it, rather than purely linear. I was reminded of it recently while reading an article on Utah's BAC (blood alcohol content) limit of 0.05. They mentioned that research shows the effect of impairment on a driver is not linear, but again somewhat quadratic, but until the 0.04-0.05 range it's low enough to not be really noticeable. But someone who has a BAC of 0.16 is functionally much more impaired - noticeably more than double - than someone just at the typical state's 0.08 limit. It's similar with PRs for me. Three 300-line PRs is less work than one 900-line PR. Even considering that breaking it up into smaller chunks might result in more net lines changed (maybe that 900-line PR becomes four 300-line PRs), I'd rather have the smaller ones.
I've tried to follow that with the Settler AI merges; it's definitely not complete, but each one makes it a little better. When there are larger merges, having notes of "there are basically three entry points: SomeClass.cs, SomeOtherClass.cs, and ThirdClass.cs", or some other indication as to the high level structure would help. Sometimes I'll break large ones down by commits, but that isn't as feasible when there are dozens of commits. And it winds up being the case where, if I'm getting paid to review those large changes, they'll get done (but more slowly than if they had been several smaller ones), but if I'm not getting paid for it, it's hard to resist making improvements to my Civ3 scenario editor, or watching an Arnold movie, instead. (Although today's choice, of enjoying the first warm weather day of the year, would have been selected no matter how appealing any potential PRs were)
Looks like the Threaded arch one is only 1349 lines when whitespace diffs are hidden, though...
.
