-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reviewing code is too long/difficult #5
Comments
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... 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've opened the discussion tab 🎉 i think it is more appropriate for such a discussion you are opening @ctmbl 😉 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 😕 |
there is also the issue with notification management... |
and the last thing, 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. PLEASE fell free to tell us anything, whether if it's lack of time, lack of motivation and even if you do not care 😉 |
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.
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 ! |
Thanks for the feedback! Next time I'll try to do shorter PR and to motivate them better |
@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 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 |
🎉 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? 🤔 |
Let's try with that in mind! 🚀 |
@amtoine |
then i pinned it and i think we can close this for now 😌 |
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:
Solutions:
these solutions/advice are not addressed to @atxr but to any maintainer on this repo or even others
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
The text was updated successfully, but these errors were encountered: