Paying more attention to PRs

[JohannesLorenz]

Motivation

Some practical examples:

• @gi0e5b06 forked LMMS, one critique was he expects PRs to be answered immediatelly (and I agree here, I'd say ideally, no "PR" should ever be open and waiting for review)
• One of my PRs (instr-sub-plugins) didn't get attention for two months, many others are still not reviewed.

Why PRs need more attention

• Coders forget what they did and has more difficulties to answer a review if it's months ago
• Coder says: "I've waited long enough, no comments, so I'll merge". Can make sense, but every bug you merge into master will
  affect others (look at our unstable master branch)

What are the reasons for PRs not reviewed?

At least:

• I often don't know when a PR is ready for review. Should this be marked in the title? Like "[ready] Some title".
• Style review sucks, it should be done by CI. We could use the clang stuff (Add .clang-format and .clang-tidy #4690) or/and some shell scripts.
• Maybe this is also a current problem of people focussing on 1.2 issues, ignoring master PRs.

[JohannesLorenz]

I think we could make PRs more attractive by

1. allowing to split style and functional
2. labelling clearly where review is missing

Maybe we could try out special labels for PRs?

• pr-wip (if it's a draft yet or the author needs to do some rework)
• pr-needs-style-review
• pr-needs-functional-review

There are also draft PRs, but we need pr-incomplete anyways for the case the author needs to do rework.

If we see it gets a fail, we can simply remove the labels 😆

[Spekular]

I'd change "reviewer" to "review" on the second two tags, and maybe pr-incomplete could be pr-wip? Cuts down on the length a bit.

…

On Sun, Apr 14, 2019, 19:48 Johannes Lorenz ***@***.***> wrote: I think we could make PRs more attractive by 1. allowing to split style and functional 2. labeling clearly where review is missing Maybe we could try out special labels for PRs? - *pr-incomplete* (if it's a draft yet or the author needs to do some rework) - *pr-needs-style-reviewer* - *pr-needs-functional-reviewer* (yes, that's still a valid label name) There are also draft PRs, but we need *pr-incomplete* anyways for the case the author needs to do rework. If we see it gets a fail, we can simply remove the labels 😆 Can I please try that out? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4935 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIgVmmXsC_MhBN1-8ss_toos9RA9EyUzks5vg2oIgaJpZM4cg8xB> .

[JohannesLorenz]

I see we already have an "in-progress" label. That looks redundant to "pr-wip", we should consider removing "pr-wip" and use the existing "in-progress".

[PhysSong]

What are the reasons for PRs not reviewed?

One of the biggest reasons is the lack of time to review PRs. I've been keeping trying to spend more time to review PRs, but I feel it's still not enough. I think many of us don't have much spare time to do reviews. We don't have enough people to compensate for it either.
Even if they(including myself) have some time, reviewing may have a lower priority for them. However, I can't find a good way to encourage or reward for reviews...

[JohannesLorenz]

Maybe we can speed up steps of the review?

Review requires 3 steps:

1. testing
2. style review
3. functional code review

Testing

There are some tests that require code know-how, but most tests do not (in my experience).

We could need some testers. Developers are very rare (as @PhysSong noted), but we have hundreds of users in Discord. With the help of the LMMS bot it's easy to test a PR. However, everytime I ask in "general" for testers, no one replies (or only devs ...).

Maybe it's not attractive to test because you won't be listed in the credits (but code/style reviewers aren't listed there, too, I think?). Or maybe there's another reason?

Style review

Will be solved by clang-format.

Functional code review

These are just thoughts, not necessarily my option. However, I wonder if we can review PRs faster, especially feature PRs. I think about Microwave (pending), Spectrum Analyzer (reviewed), or Lv2 (reviewed). E.g. when reviewing Spectrum analyzer, I tried to understand every code line. Maybe this is too much (I don't know how others do reviews).

What must be reviewed:

• Bug-Fixes on feature-freeze branches (like stable-1.2)
• Audio, if it can damage your ears/speakers (but maybe it can be tested)
• Compatibility relevant parts (e.g. savefile layout, because this gets ugly to fix later)
• No compatibility breaks

Maybe, the following must not (sorry, wrong English) does not need to be reviewed?

• Almost any UI code (it will be seen by testers, and it has not comatibility issues - if it's wrong, fix it in a later PR)
• Perfect style:
  • It's better to use a unique_ptr than a normal pointer. But could that not go into a checklist?
  • E.g. "Can this macro be replaced by a function?". If the reviewer sees it, they can give a hint to the dev. But the code will mostly work with macros, too. Do we need brilliant code that will never get merged, or good code that will get merged (and gets more testing on master)?

It could be even more easier if the dev checks the "What must be reviewed" list and passes it to the reviewer.

[JohannesLorenz]

About Testing: We now have an experimental "#testing" Channel in Discord. Thanks to @he29-net for the idea and @Umcaruje for setting it up!

[michaelgregorius]

I'd like to add own experience here as well as I have gone through several code reviews in the last months.

First of all I'd like to thank everyone who has taken the time to give me a review! I am aware that I am by far not doing as many reviews as others. However, I also consider myself a little bit more "remote" to the project due to the fact that I am not on Discord and therefore don't really know what's going on in the project. Another reason is that I do not have that much time for the project anyway and therefore I'd like to concentrate more on providing features. I am also aware that this would not work if everybody did it like this. 😉

To much focus on style reviews

From my experience I'd say that currently far too much focus is put on style reviews. Or perhaps I should rather say that I am under the impression that I am mostly getting style reviews and almost no functional reviews. I am not saying that a coherent style is unimportant. However, it will never get 100% coherent anyway and style reviews should also not be about personal little preferences in my opinion.

To me it also does not make sense to get style reviews from different people. The reason is that they all have different tastes and preferences. Due to that it always feels a little bit like the style of the corresponding reviewer is somewhat "forced" onto the own style before it is green-lit. So I agree that the code styling should be done by a tool. This would at least be more coherent. Alternatively the style reviews should be more "forgiving" with regards to personal taste and only report more "serious" issues.

Functionality and Features

In my opinion the functionality of the application and getting it forward with regards to new features should be more important than the code style anyway. Because the functionality is what the users are actually seeing (and hopefully appreciating).

[Veratil]

To much focus on style reviews

I'm around more than others which can chime in better on functionality, and my focus is style first anyway. The clang-format/tidy configuration we do have isn't 100% compliant with the way our coding conventions are, and we generally need to decide if we want to change the convention or adjust the tooling. Eventually style reviews will just be checking if you ran tidy/format, but until that day we still need to try to maintain convention throughout the codebase as it gets updated. The opinionated reviews on certain style formats are because we don't have convention for that situation, and we need to decide how we want it to be formatted for when we use style tooling in the future.