Get our pull request game in order

Piedone commented 8 months ago

Is your feature request related to a problem? Please describe.

We have ~190 open PRs currently, with many from years ago (22 of these are dontmerge/notready, and an additional 20 are drafts. While we have PRs being closed continuously, still, this is a large number that I think we should do something with. Also, and especially for external contributors, we should close PRs quickly (including merging or rejecting with constructive feedback).

This would benefit the community, and inspire more people to contribute (more).

Describe the solution you'd like

[x] Have tips for reviewers. E.g. did you know that here's a list of open PRs reviewed by you? You should maybe check them because perhaps it's time to ping the authors since maybe they forgot to follow up? Or maybe you forgot to merge a PR? (While checking out older PRs today, I've seen plenty of both. We could also auto-merge with e.g. https://github.com/reitermarkus/automerge.) - https://github.com/OrchardCMS/OrchardCore/pull/15706
[x] Maybe auto-close stale PRs after some time if the author didn't follow up after a review? So we don't have a long list of abandoned PRs. E.g. with https://github.com/actions/stale. - https://github.com/OrchardCMS/OrchardCore/pull/15710
[x] Standardize what means "this PR is a work in progress, don't review it unless I ask you for your feedback, and for sure don't merge it". Currently, we have three overlapping ways to do this and it's confusing when looking for PRs to review: the dontmerge and notready labels, and draft PRs (and if you want to be really sure, you use all three :)). I propose settling on the latter, since that's a built-in GH feature. "dontmerge" can remain in a documented way for PRs that are ready for review, approved, but you want others to approve too. Similarly, we can remove the "ready" label. - Removed the "ready" and "notready" labels, documented the rest. - https://github.com/OrchardCMS/OrchardCore/pull/15706
[x] Document the usage of the Needs Triage label. When you add it, also add a comment why do you want it to get triaged, what are the open questions, what are you unsure about and looking feedback for. - https://github.com/OrchardCMS/OrchardCore/pull/15706
[x] Get our CODEOWNERS updated because it's almost all outdated now. Or rather remove it: Collective code ownership is better, and the supposed code owners mostly just ignored these automated review requests anyway (and it confuses contributors). - https://github.com/OrchardCMS/OrchardCore/pull/15692
[x] Label a person's first PR (or the first few) so we know to be especially gentle, patient, and encouraging with them. See e.g. https://github.com/actions/first-interaction. GH actually provides an indicator for this, so this might be not required: - https://github.com/OrchardCMS/OrchardCore/pull/15708
[x] Ask reviewers to actually merge PRs that they approve for external contributors, not to keep them hanging, unless they want a second opinion, but then ask specific people for that. - https://github.com/OrchardCMS/OrchardCore/pull/15706
[x] Make contributors aware re-request reviews once they're done. If a PR had changes requested before, and there were commits since then, and a week passed, then add such a comment automatically. Such tips can be added as bot comments or with PR templates (e.g. AWS SDK for .NET has a good one, just as ImageSharp.Web). - A simple PR template will suffice for now: https://github.com/OrchardCMS/OrchardCore/pull/15706
```
Please click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.
```
[x] Ask more people to review. It's not as sexy as contributing code, perhaps, but it's still awesome! And you get green blobs for it under your GitHub profile too, wohoo! I've reviewed 8 PRs today after a sudden rush of motivation, and I promise to do reviews with a more reliable cadence.
[x] Ask contributors not to resolve PR comments, because then the reviewer will need to open them one by one, to remember what they asked for, and to see if everything was addressed: - https://github.com/OrchardCMS/OrchardCore/pull/15706
Apply code suggestions directly so the reviewer doesn't have to eyeball the changes. These resolve themselves after applying them, and that's fine.
Don't resolve other conversations so it's easier to track for the reviewer. Then, the reviewer will resolve them.
Feel free to mark conversations that you addressed to keep track of them with an emoji or otherwise, just don't resolve them.
Please keep conversations happening in line comments in those convos, otherwise communication will be a mess. If you have trouble finding them, see this video.
[x] Ask contributors to also add themselves as contributors: https://allcontributors.org/docs/en/bot/installation#4-update-your-contributing-documentation. This can be done automatically as well with e.g. https://github.com/actions/first-interaction. - https://github.com/OrchardCMS/OrchardCore/pull/15708
[x] Ask contributors to reference the corresponding issue with Fixes #<issue ID>, see the GitHub docs. Ask them to open issues for all but trivial changes so we can discuss whether it's a good idea first before putting a lot of effort into it. Alternatively, describe the context and rationale in the pull request description, and provide screenshots/screen recordings of the changes if they affect the UX. Refactoring is great, but if you do so, please guard it with new tests. - https://github.com/OrchardCMS/OrchardCore/pull/15706
[x] Document all of the above. We have the CONTRIBUTING.md file, but I'd move its content out to the docs site (similar to this page to have something perhaps more appealing, and link it from the MD file. Also ask people to update the upcoming release notes. - https://github.com/OrchardCMS/OrchardCore/pull/15706

After these are done, all open PRs need to be merged to (what core contributors can do with a click from the PR screen or ask the author to merge and resolve conflicts) to get the new automation, if those come from GitHub Actions.

Describe alternatives you've considered

What we have currently is not bad, just it could be better. I'm also testing AI code reviews by https://coderabbit.ai/. These are free for open-source and might help us; or might just provide trivial noise, I'm not sure yet, but I'll get back here if it's useful.

After tackling PRs, perhaps we should improve something in issue management too.

MikeAlhayek commented 8 months ago

Agreed. Having 100+ PR that are just sitting there is discouraging new contributions.

Also, have 1000+ issues is just as bad. We should auto convert any "how to question" to a QA discussion. Any issue we are not going to consider should be closed not just tagged with backlog. This was we can clean up issues and hopefully we have a shorter list people are willing to cherry pick from and fix via PR.

Piedone commented 8 months ago

Yep, agree.

Piedone commented 8 months ago

Opened https://github.com/OrchardCMS/OrchardCore/issues/15034 for issues.

Also:

[ ] Ask contributors not to resolve PR comments, because then the reviewer will need to open them one by one, to remember what they asked for, and to see if everything was addressed.
[ ] Label a person's first PR (or the first few) so we know to be especially gentle, patient, and encouraging with them.

Piedone commented 8 months ago

And now there are only 180 PRs open. With this pace, we'll get to 0 by the end of January :D.

Piedone commented 8 months ago

This looks useful for PRs too: https://github.com/OrchardCMS/OrchardCore/issues/14585.

Piedone commented 8 months ago

@hishamco please go through your draft PRs and check whether something can be closed because you won't revisit them. You also have a lot of dontmerge PRs

If anything is useful from these PRs even after a close, then please create issues.

Piedone commented 8 months ago

If you have any feedback/objection here, especially those recently active core contributors who haven't chimed in here, please do @Skrypt @Skrypt @sebastienros @kevinchalet @deanmarcussen @ns8482e @agriffard. Otherwise, I'll just work through these points one PR each. Same for https://github.com/OrchardCMS/OrchardCore/issues/15034.

kevinchalet commented 8 months ago

@Piedone sounds like a good plan (no objection for removing code owners if you think it's better).

Otherwise, I'll just work through these points one PR each. Same for #15034.

I took a look at the 2 OpenID-related PRs and I think one of them can be closed. The other one - that updates the OpenID module to use the OpenIddict client instead of the MSFT OIDC handler - should probably wait until the secrets module is ready.

On a related note, that would be nice to clean the Git branches up... because with ~160 branches (5x more than dotnet/runtime!), it's honestly a giant mess 😄 Encouraging core contributors to keep these branches in their own forks instead of adding them to the main repository would be a very effective way to reduce the number of branches but I'm not sure it will be a popular change 🤣 IMO, having feature branches in the main repo only makes sense for features where multiple contributors are expected to work on at the same time.

Piedone commented 8 months ago

Sorry, by "I'll just work through these points one PR each" I mean that I'll open PRs to implement the changes for the bullet points of the issue (not to have all at once which is harder to review). But I do also work through the old PRs :).

As for branches: they don't bother me personally (since there's no UI or operation that would be worse for me if the number of branches increase), where do you see the issue with them? We could delete branches after PR merges, though that would remove the history (since we merge with squash merged, what I don't like BTW :D), or remove everything for unmerged closed PRs. Everybody working in forks would make collaboration harder indeed (not impossible, since core contributors can push to even forks).

kevinchalet commented 8 months ago

As for branches: they don't bother me personally (since there's no UI or operation that would be worse for me if the number of branches increase), where do you see the issue with them?

It causes a lot of noise and makes finding relevant branches very hard. E.g in GitHub Desktop:

since we merge with squash merged, what I don't like BTW :D

Squash merge FTW! (or rebase merge on a PR with a few commits when it makes sense to keep them separate) 😄 If you need the history, you can still see it on each PR (unless you manually rebased/squashed your PR before merging it).

Everybody working in forks would make collaboration harder indeed (not impossible, since core contributors can push to even forks).

Well, it's an approach used in very active repos like dotnet/runtime, so I'm not sure it really makes it harder (and when you think about it, most PRs includes commits pushed by a single author).

Piedone commented 8 months ago

I see. I guess why such long lists haven't really bothered me is that I always search for what I'm looking for (which is usually copied from e.g. the PR anyway).

MikeAlhayek commented 8 months ago

@Piedone, @hishamco has the power to drop the total open PRs to under 100 if he close or merges his 58 open PRs :)

Hisham, I suggest merging any PR that solves a problem. If it is just a refactoring and does not solve a real issue, maybe see if we really need it and then either merge or close. personally, I feel refactoring PR mostly don't add much value but they add the risk of introducing bugs specially if there aren't any test cases. So I suggest If you want to submit a refactoring PR, you should also add all the needed test cases as part of refactoring PR otherwise the risk outweigh the reward.

hishamco commented 8 months ago

I will have a look at all of them, no worry I knew I had many PRs because I'm too old :) another issue was merging PR before +3 years are not easy as today

Piedone commented 8 months ago

Yeah, I already asked Hisham above :).

hishamco commented 8 months ago

I'm struggling to revise the old PRs, and then close or merge them. Don't forget that they took long time to review :)

Skrypt commented 8 months ago

The point is that from past experience there was simply not enough time to review all of them in a single 1 hour meeting per week with @sebastienros (no offense)

Also, reviewing PR's sometimes will take more or less time depending on the size of them. Sometimes it will discourage new contributors because over time our acceptance level has raised. I agree that some PR's can be merged even if they are not feature complete but sometimes this is where the PR's become in a stale state; when the contributors are not applying the proposed changes to their PR. So, from experience, it is the main contributors that often requires to take these PR's and complete them... just like Jean-Thierry did all these years.

I personnally can't contribute as much as I was because of my time employment. I think we need to find more time to simply review PR's and decide which one we will simply close based on contributors and time last updated. The thing is that we need also to prioritize on features upgrades like the Newtonsoft to System.Text.Json one... the project needs a clear and precise list of things TODO just like we had when we we're developping before 1.0.

Stop merging every PR's that adds new features and make a plan for external module support instead. We keep adding things in OC while we need to make it lighter and more like a framework, the task will just be harder if we keep doing what we've been doing.

Example of that is the many Search modules which could be totally external modules but that we ended up pushing in OC because as dev we all want to have all the options available. 😄

hishamco commented 8 months ago

I agree with you @Skrypt OC has become too BIG :) that's why I started Orchard Core Contrib (OCC) but I'm the only maintainer :( Also, Lombiq did a great job. IMHO we should support the community around Orchard Core and let us focus on the stability and cleaning of what we already did

One more thing I'd like to mention and push from years, we need UI improvements I have already some attempts in the past. OC seems designed by devs :) In my entire career, I have seen many frameworks & CMS put some effort into the UI design to make the UI look pretty

Hope to open a discussion to discuss the future and the plan for OC

Skrypt commented 8 months ago

And yeah, I agree that mostly what we need to do is sync with @hishamco to review his 58 pull requests which is 1/3 of them once we will have taken care of smaller ones.

Sorry @hishamco about that.

hishamco commented 8 months ago

No problem I'm struggling to merge what I can, also I have a broom to clean what I can :)

Skrypt commented 8 months ago

We need a Github background task to do a gc.collect on @hishamco PR's 😉 Just kidding 😄

hishamco commented 8 months ago

Also I need a task scheduler to remind you and Seb to approve any localization related PR :)

lol

Piedone commented 5 months ago

OMG, we're below 100!

hishamco commented 5 months ago

We need a Github background task to do a gc.collect on @hishamco PR's 😉

I ran a background job on behalf of @Skrypt :)

Piedone commented 5 months ago

Awesome :D.

Piedone commented 5 months ago

I guess you're doing this already, but if not, @sebastienros on the Thursday triage meeting please check on the Needs Triage PRs. The other PRs are not necessary to check out there for now (unless there's time, of course).

Piedone commented 5 months ago

I'm done going through every open non-draft PR.

I've done reviews, closed with or without merge as applicable, chased up people. Within a few weeks, we should get down to a manageable 50 open PRs, and stay there. (The goal is not zero, but to have only active PRs open, and to give new PRs feedback ASAP.)

I'll follow up with implementing what's under the issue above.

MikeAlhayek commented 5 months ago

Great work @Piedone! Great progress.

Piedone commented 5 months ago

Thanks!

Piedone commented 5 months ago

Please check out the PRs linked in the issue description.

Piedone commented 5 months ago

Summary or PRs:

Piedone commented 4 months ago

We're down to 69 PRs! (nice)

Piedone commented 4 months ago

On to https://github.com/OrchardCMS/OrchardCore/issues/15439 and https://github.com/OrchardCMS/OrchardCore/issues/15034!

Piedone commented 4 months ago

60 is so close!

MikeAlhayek commented 4 months ago

closing the Rabbit PR will make it 59. just saying lol

Piedone commented 4 months ago

OrchardCMS / OrchardCore