+- Cuberite Forum (https://forum.cuberite.org)
+-- Forum: Cuberite (https://forum.cuberite.org/forum-4.html)
+--- Forum: Development (https://forum.cuberite.org/forum-13.html)
+--- Thread: Pull requests remain open for too long (/thread-2363.html)
RE: Pull requests remain open for too long - NiLSPACE - 03-03-2016
Yes, it's pretty bad that we leave so many pull requests open, I mean, we even have a PR from May 19 left open. I also agree that we should wait telling people to rebase until we're sure all the issues are fixed. I think the 5 spaces between functions is a matter of preference. I personally like it, they make the code look more organized. Perhaps there are people who agree with you though. If you want you can create a thread to discuss on reducing the amount white spaces between functions.
I hope you can understand that those comments are all to prevent PR's like this one and this one
RE: Pull requests remain open for too long - warmist - 03-03-2016
(03-03-2016, 05:54 PM)NiLSPACE Wrote: I hope you can understand that those comments are all to prevent PR's like this one and this one
Yeah i understand it totally. I guess i'm just annoyed by lack of progress in the things i want.
RE: Pull requests remain open for too long - sphinxc0re - 03-03-2016
And again: @warmist I'm sorry for losing my attitude but people are complaining about left open PRs so often whilst most of these left open PRs have been abandoned by their authors. Also at the meetup we decided to adjust our reviewing process so PRs won't be left open for so long.
Future PRs will be closed as soon as the code is clearly unusable for the project. Of course we will provide a lot of comments for the author.
RE: Pull requests remain open for too long - LogicParrot - 03-27-2016
Quote:I'm not saying it was awful. Just the general no-reaction whatsoever interspaced with one comment about one line... i get enough of that from my day job and having to adhere to yet another set of arbitrary rules (5 spaces between functions?! really? so that you could never see two functions and never understand any links between?
I can see your point. I often see perfectly good PRs that receive several comments, where almost all of the comments are style related, and not logic-related. While I believe style guidelines are very important, I can see how this can be energy-sapping, and can give the impression that "these devs are nitpicky and only care about eye-candy, and I think they did not even review my code logic, they just hunt for my style problems". Sometimes this is the feeling I had when I started contributing. While it did not really bother me, I can see how it can bother others.
One middle-ground solution which does not involve relaxing the style would be not mentioning a PR's style issues until all logic problems are commented upon, and all non-style code issues are fixed. Once the crux of the PR is declared good, style comments are to be made. One could establish a simple rule: "Never comment on style problems unless one of the devs declares this PR's logic good.". This might help dissipate the perceived "no" atmosphere that the style reviews inevitably create, and each good PR would receive at least one positive "looks-good" comment even if it has style issues.
Of course, edge cases where the style is absolutely horrible and makes the eyes bleed should not be logic-reviewed before the code is put into at least a readable state. And in this case ranting about the style before checking the logic is acceptable.
This might be completely unnecessary and/or ineffective, I am just thinking out loud.
Edit: As a case in point, my latest PR just got a style comment. This is a very typical occurrence. While this is a perfectly valid style comment, I can imagine how this can negatively affect a newbie, especially when said newbie has submitted far heavier changes and many such comments are received prior to anyone doing code-logic review.
RE: Pull requests remain open for too long - sphinxc0re - 03-28-2016
@LogicParrot the behavior you're suggesting is probably a drift in the right direction. Although I believe that our whole reviewprocess needs to be reviewed soon
RE: Pull requests remain open for too long - LogicParrot - 03-28-2016
(02-14-2016, 09:26 AM)tigerw Wrote: Is this a problem which needs fixing? Eventually someone will get around to them; if not the author, one of the maintainers.
Maybe not. Maybe yes. I've observed abandoned PRs (that nobody got to) and I thought it's at least worth discussing. I don't know if this needs fixing.
RE: Pull requests remain open for too long - Schwertspize - 03-31-2016
As I figured out we are now closing many of these old prs.... Is this an acceptable solution? (AFAIK they are being closed because they are not active any longer)
RE: Pull requests remain open for too long - NiLSPACE - 03-31-2016
If PR's are too old they will eventually have conflicts with the master, so yes, it's better to close them in this case.
RE: Pull requests remain open for too long - tigerw - 04-01-2016
We could employ @TayTweets to review our PRs.
RE: Pull requests remain open for too long - NiLSPACE - 04-01-2016
Isn't that the bot by Microsoft that was rasist because it learned from other people?