Reviewing code is too long/difficult

[ctmbl]

Context:

Recently @atxr opened two amazing PR (#1 and #2) to deploy the first version of the iScsc blog.

Issue:

The reviewing step is taking faaaaaaar too long (almost 1 month for me to review #1 maybe 15 days to merge it, same for #2).

Causes:

I personally identified 3 root causes:

• these PR are quite long (65 commits for #1 38 for #2 if I'm right) --> I hope this is because there are needed to write the blog structure
• none of us are familiar with React neither are expert in JS or simply web coding --> we don't know were to start

• we miss documentation, context and concrete facts to imagine the code we're reviewing in action and identify flaws

  Solutions:

  these solutions/advice are not addressed to @atxr but to any maintainer on this repo or even others

  • try as much as possible to write shorter PR, even if it means merging incomplete code like models and controllers which aren't useful at the time
  • better introduce the PR, especially because there are the first/they introduce new concepts/framework!! Try to explain them to us and maybe link images
  • actual commits are really good but try as much as possible (once again we're still noobs) to detailed your commit bodies that can be really helpful!
  • send links/documentation that address the PR to help us understanding it
  • and the MOST important. Reviewers: try to DO your JOB please. I'm the first to lack time and to take long to review code but reviewing is as boring and difficult as it is vital and encouraging for the developer. For the sake of developers motivation and this project future please review.

  My real point:

  First of all, the long reviewing time is an insidious flaw because it not only lower the maintainer motivation but also drive away the reviewers from their duty AND the project. Taking time to review delays a PR merging but also next features development! Please think about it, not reviewing code is killing the project!

  Moreover when you're on the developer side it's so amazing and motivating to have feedback and review, we're all on both side so do it and others will!

  However another problem arisen to me when reviewing @atxr PRs. #2 introduces user auth, and we can't even take the time to review this widely critical features, because we deeply miss the expertise to do it right. Let me write it straight. We are not able to develop and review really safe code because we aren't cyber-security experts.

Then what do we do? I'll open another Issue to discuss this deeply different problem.

I think that this Issue concerns everyone so here I go: @atxr @amtoine @J3y0 @Turtyo

[atxr]

Thank you for this issue @ctmbl

You are totally right, the long delays for reviews is killing softly the project and the motivation of the developers... On another hand, I can totally understand the fact that, as reviewers, you have other projects, and you don't have time to "waste" in these reviews. It's actually a very hard task because most of the time when reviewing, you have neither the context, nor the skills sometimes (js for example in this project). And you can also don't give a f*** to the project :eyes:

Nevertheless, I want to highlight the importance of reviews. Typos, bad code, buggy features, bad design... All those points are very hard to spot as a developer, and talking about what you did with reviewers helps to correct them.

I tried to assign some tasks during #2 (check the JWT for @J3y0 and @Turtyo for example) to propose some guidelines, tell me if it helps. I also thought about another way to review the code. For big/slow PR like that, we could organize PR meetings where the developer presents the new features, critical points... If it can be easier for reviewers to find the motivation to make the first step, I'll be glad to do it! And it can be a nice way to present the technologies involved in the project. These meetings could be public (with other interested members of the club) to have an overview of ongoing projects in the club.

[amtoine]

i've opened the discussion tab :tada:

i think it is more appropriate for such a discussion you are opening @ctmbl :wink:

tbh, i think the problem is really the lack of motivation of the contributors but i do not really have ideas to change that right now :confused: documentation is always great and why not PR meetings, but in the end there is still this deeper problem of motivation and lack of time :thinking:

[amtoine]

there is also the issue with notification management... we can not force them to do so but people have to manage them and themselves to (1) check them, (2) treat them and, most importantly (3) not let them pile up and (4) not let them disappear

[amtoine]

and the last thing, contributors have to understand that they CAN lack time when there are reviews to do, that is 100% legit :+1:

BUT and that is a big BUT, you can NOT freeze whole PRs with status quo and saying nothing about the work the others, especially @atxr, do in the background. you HAVE to tell us when you lack time, in order to move the attention to contributors that might have some more but get unnoticed 'cause the attention is focused on this lack of time

PLEASE fell free to tell us anything, whether if it's lack of time, lack of motivation and even if you do not care :wink:

[Turtyo]

try as much as possible to write shorter PR, even if it means merging incomplete code like models and controllers which aren't useful at the time

for me this is the biggest point, I think it would be much easier to review more but shorter PR than to review a very big PR once a while. Two reasons for that, first of all since it's shorter it feels easier to read, and second, if we review regularly it keeps us in the project, instead of having to understand everything right away and forgetting about it between PR.

I also thought about another way to review the code. For big/slow PR like that, we could organize PR meetings where the developer presents the new features, critical points...

That could be a good idea, but it wouldn't be needed as much if the PR were smaller I think.

I think the lack of motivation mentioned throughout the discussion also comes from the fact that it's not something we do regularly, it's not a habit and it feels more like a task if you do it irregularly than if you do it every week or every day. Which again, could be fixed with shorter PR !

[atxr]

Thanks for the feedback! Next time I'll try to do shorter PR and to motivate them better

[ctmbl]

we can not force them to do so but people have to manage them and themselves to (1) check them, (2) treat them and, most importantly (3) not let them pile up and (4) not let them disappear

@amtoine you're absolutely right here. @Turtyo @gbrivady (new comer) @J3y0 I don't know if you use GitHub notifications but if not start using it!

BUT and that is a big BUT, you can NOT freeze whole PRs with status quo and saying nothing about the work the others, especially @atxr, do in the background. you HAVE to tell us when you lack time, in order to move the attention to contributors that might have some more but get unnoticed 'cause the attention is focused on this lack of time

But here is the real point @amtoine 's got! Please let everyone know when you're available, when you're not, when you lack time or motivation and also give deadlines!

As for the rest, every developer should aim to propose small PR, it seems to be a real issue here too! Making reviewing common would be great @Turtyo is right here

[amtoine]

:tada:

so now, is there anything really we can do apart making sure everyone says when time is or is not there and all notifications are not lost in nothingness? :thinking:

[atxr]

Let's try with that in mind! :rocket:

[ctmbl]

@amtoine No we can't really do anything, but I hope that this issue will act as a reminder of what we have to do for these projects to persist and thrive! And talking about it was necessary too!

[amtoine]

then i pinned it and i think we can close this for now :relieved: