Getting stuff done (closed)

remoteStorage.js
1

There is this PR which tries to do a super-tiny thing, and that now has been adjusted 14 times since it was first created. It was rebased twice. More than 60 (!) comments were made back-and-forth, over the course of four whole weeks. I was the person submiting it, and I did everything everybody wanted, yet it’s still not merged.

This branch now contains not only its original purpose (exposing the properties that come from WebFinger, in itself a very easy thing), but also:

  • a change in function signature of 5 or 6 different functions,
  • new tests for code paths we weren’t testing before,
  • new documentation,
  • a switch from callbacks to promises in some parts of the library,
  • all the “whitespace wishes” of four different reviewers.

Yet so far it has only been +1’ed by one of them.

Looking at how super-tiny the original commit was, and the scope of just “expose WebFinger link properies as remoteStorage.remote.properties”, am I the only one who finds that maybe we’re going too far with this?

I’m not saying refactoring is a bad thing, and I understand how the piggybacking/blackmailing works, so we force ourselves to “clean as we go”. Sure.

And I don’t think this is anybody’s fault, I just think our work process has slowly slided into a bit of a paper-shifting culture, while everybody is trying to make it better. This is a classic thing to happen as any project progresses and grows, I guess.

But let’s force ourselves to stay a bit more agile. In the future, I think we should separate out month-long refactoring discussions like this from just getting stuff done. Whenever there is something wrong with the actual change, as a reviewer, you should say so. If not, you should +1 it, or just say “+1 if you fix X, and add a comment explaining Y, since that was not clear to me at first sight”.

If reviewing the PR makes you look at code that was written 2 years ago and which you normally never look at, then chances are you’ll find stuff that can be improved. That’s good, but just open a new ticket about it then. Otherwise we can never get stuff done.

Even if we just start giving more “+1 if …” reviews, it would already be a big win in speed, because a coder who gets two “+1 if …” reviews can apply the suggestions during merge, and it means a PR only goes back and forth once.

Also, it would help to shine the spotlight more on “we, the people who write the actual code”, and a bit less on “we, the reviewers”. Even though I consider myself part of both groups, I feel we have to fight for a bit of telecommunism here. Wer baut hat Recht! :slight_smile:

2

Uhm, the quality of that code greatly increased, and no matter if you write it or suggest changes, you’re helping to create a better outcome. You let it sit there yourself for a long time, not rebasing it and actively trying to avoid rebasing and making it ready for a last review.

Please ping us on IRC for discussing these things. Everybody in that PR reviewed rather quickly actually.

3

Also: lots of change suggestion from reviewers means a lot of people cared and actually had ideas for how to improve the code there. How can that be a bad thing? Just because it takes a little more time doesn’t mean we should actively try to rush in changes for the sake of being faster in my opinion. It was like that before we introduced the QA process and it didn’t work well in the end. We’re still paying that technical debt today.

4

Having ideas is not a bad thing, but it doesn’t move the needle either. What matters more is people who actually do refactoring, like @silverbucket’s bluebird and nodejs refactors.

That’s why I called this topic “getting stuff done”. By spending less time reviewing back-and-forth, we can be more agile as in, spend more time actually doing things and fixing things. That includes things like refactoring, documenting, and writing tests.

5

Although I understand your frustration @michielbdejong (I usually have a few pull requests pending as well / have to tweak and resubmit etc.), I really do think that our code (both mine and yours and anyone else submitting a PR) is improving measurably during the course of the PR lifecyle.

I know it can be a pain (I’m always tripping over my own commits and trying to battle between the verbosity of my normal commit / change process vs. the desired concise and brief commit history that’s set as the standard for mergd PRs) but the alternative is that more things are going to slip through the cracks, there will be more issues filed as “todo/someday” for exact pieces of code that were touched by the PR submitter. We all know the reality of the situation is that those issues will pile up and only a small fraction of them will be addressed.

While it makes sense to file issues for changes you’d like to make / ideas you have when reading through the code (or discussing it in IRC). I don’t think it makes sense to accept this approach as a standard way to avoid making changes to code that you are actually touching right then unless those changes imply significant work in other areas as well.

As for the idea of “+1 if you change X and Y” this leaves room for very bad things to happen with merge rebasing / changes, misunderstood wording, laziness or creating bugs inadvertently etc. We all have forgotten/overlooked things when changing code to address someones feedback, so it really doesn’t make sense to make this “automatic” so to speak.

That said, I do think we need more prompt review of code we already took a look at and suggested changes for. There’s no reason @michielbdejong’s PR should be so heavily reviewed by multiple people, and 2 rebases, only to sit now with a single +1 and no additional comments. It’s demoralizing to put work into a bug or feature, spend time tweaking based on feedback, rebasing, etc. only to have it sit there and grow stale (increasing the likelyhood of a new rebase needed when you finally get your +1’s to merge).

If we’ve reviewed a piece of code, we owe it to the submitter to be prompt about the new round of reviews and/or just leave a note saying we can’t look at it until X time.

6

That exact PR was updated yesterday to improve code based on good suggestions by @galfert. 1 (one) day ago.

Yes, we need to be fast, but you cannot expect a team of voluntary contributors to be able to review things on the same day every time for stuff that needs to be understood and tested.

I understand the frustration, but let’s be reasonable about turnaround time expectations.

7

Actually, looking at the history again, the PR picked up pace during its lifetime and in the end all reviews were done on the same day as the additional commits. My last one even includes a new commit I made myself instead of commenting and waiting for you to fix the docs. So I really don’t see where this is coming from, as we’re already getting things done in there. Yes, it’s a long-running PR, but submitting broken docs or unnecessary code just so it lands in master faster is exactly what the process was intended to prevent. And that’s what it just did. With relatively good turnaround times at that.

8

I can’t explain it any better, sorry. I’m glad that at least we all recognize that our current process can sometimes be frustrating for the contributors and volunteers involved. Let’s all try to keep an eye on that.

I also think our process is currently inefficient and can be improved in that sense, too (I even think even very inefficient, but maybe that’s a personal perspective).

Let’s conclude that each contributor can see for themselves where they can help to improve the efficiency of the process as a whole to keep things rolling smoothly, and make sure they are not blocking the process for other contributors.

Improving efficiency will hopefully also help to reduce contributor frustration.

9

If you have new perspectives and know about things we don’t, then please bring them forward into the conversation, so we can try to find the best way! If you know exactly what’s the most inefficient, then it shouldn’t be hard to demonstrate that. As it turned out during this discussion, it wasn’t exactly the things you desribed in the original post, so where are these mysterious huge inefficiencies?

10

replied via irc

11

… and dropped out of it right away. No new insights to this topic. Doesn’t help progress towards a better process. If we can improve it, let’s do it. “Getting stuff done” includes all aspects of the project, including process design and other non-programming topics. Blanket statements don’t help us make progress towards a better situation than we have today. Taking things personally doesn’t help to keep the discussion objective and useful.

12

Just to clarify, if it’s not clear from the discussion: please propose specific changes to the QA process of rs.js, so we can have a productive discussion about them and have everybody add and weigh pros and cons of the change! That is the only way we can improve it, based on the best possible input from all sides and consensus about it.

13

ok, so one useful amendment would for instance be that only the first round is allowed to demand changes to code quality (style, documentation, test coverage, variable naming, use of functions, and other remarks that are aimed at keeping and improving code quality).

two reviewers can participate in this first round if two people are available, but if only one is (i.e. the second reviewer arrives after the submitter posted “ok, i will make those code quality changes”), then the second reviewer reviews just for

a) does the PR fix the issue it’s supposed to fix (and is this issue worth fixing)
b) does it do so in a functionally good way (and does not cause problems elsewhere or later)
c) does neither the original PR code nor the code quality refactor work introduce new bugs

So we still review for code quality, but only once per review process, and not in a potentially infinite loop. Once the demands have been formulated, there is a “refactor freeze”, and the rest of the review process is only about those other points a, b, and c.

Of course it can (and will) happen that a reviewer sees more room for improvement, or that it wasn’t clear from the refactor freeze what was desired. then there are two situations:

  • if the submitter messed it up, then they still need to try again (but the refactor freeeze stays the same and can not be expanded to include more things than it did originally).
  • if they did what they could to address every point specified in the refactor freeze, and did not introduce new bugs, then the review has to pass. any further improvements have to be postponed to a new PR.

So requirements for a PR to pass review would be:

  • it has a refactor spec from the first reviewer
  • the refactor spec has been implemented
  • two reviewers agree with the submitter that this PR solves something worth solving
  • two reviewers agree with the submitter that this PR solves what it solves in a functionally good way
  • two reviewers checked that the refactored PR does not introduce any bugs
14

a second improvement on top of that would be to say that reviewers should try to constantly (after each ping from the submitter) reply with one of the following answers:

  • code quality spec (this type of review comment can only be made once, see above)
  • -1 (meaning you think the whole PR should be discarded)
  • bug (meaning the PR introduces a bug)
  • +1
  • I don’t have time during the next three days (meaning another reviewer can take over)

If a reviewer doesn’t reply to a ping for whatever reason then that can be interpreted as this last case.

15

Is that your idea of having a simpler, less bureaucratic process?! This sounds horribly complex.

I want to get stuff done, not think about in which of your 5 different situation loops I am currently.

16

I just read this whole thing two more times. This is so terribly complex, that I wouldn’t want to submit code changes myself anymore, and even less participate in this bureaucratic review process. This is the opposite of how I’d think we can solve the potential problem of reviews running too long.

Here’s a simple idea: if you’re waiting for people to review stuff, because you’re in a hurry or something: contact people, e.g. on IRC. Another simple one: if refactoring doesn’t make sense in a certain spot or is preventing getting stuff done: say that and it can be decided case by case.

In fact, I think that just communicating with your fellow humans can solve everything these new bureaucratic rules try to solve, without having a single process amendmend. Let alone turning it from “needs two +1s” into “needs x, but only in case of y, and if z, then a, but only if b is not c”.

17

thanks for your feedback. i don’t think it’s complicated to say code quality review is done only once. but we can make it an unwritten rule - just try to give all code quality feedback right at the beginning (in the first review round), and only add additional rounds if bugs appeared.

18

That doesn’t make sense as a general rule. The whole point of a PR is editing and potentially adding more code in the process, and that code should then obviously be good as well.

19

I think of the things you’re thinking about wrong is the concept of “rounds”. When doing pull requests the right way, there are no rounds and it rarely happens that someone objects to merging after the first +1, when all concerns have been addressed and they don’t have a very good reason for it. The whole point of having two +1s, though, is that one person can easily overlook things, whereas four eyes just see more.

20

No, that’s where you’re wrong, and please stop promoting this idea because you’re not helping the project. It’s exactly what is so inefficient. Editing and adding code happens after opening/reproducing the issue, and before creating the PR.

The PR is for review, and a good review gives all feedback immediately. Once this feedback is taken into account, and unless new bugs/typos/problems were introduced, no new feedback should be added, and the PR should be merged. I don’t know why you find this so complex. New ideas for more improvements can be put in new tickets (called follow-ups) that lead to new PRs. Simple! :slight_smile: But we’re very far away from that ideal situation.

If that were true then the PR I gave as an example here would have been merged after this first round. What happened instead is what I described above.

I think it is clear to everybody that this situation needs to improve. Let’s stop talking about it, and just all try to get better at it from now on.