Contributors mailing list archives

Browse archives


Re: Reviews

Ecosoft Co. Ltd., Kitti Upariphutthiphong
- 16/09/2021 18:46:00
Hi Tom,

Thanks for bringing this up. I experience the same. May I share opinion as a not so advanced developer side.

If I understand correctly, the policy of OCA is to welcome more contributions with maturity levels like alpha. If this is the policy, we will have a lot of PR (with many low quality ones too). I think good / interesting PR are still merged fast, and natural selection will kill the low quality module in the long term anyway.

So, frankly, I don't mind having a lot of new PRs, even unmerged ones. When I got stuck, they also useful.

But I like to support some idea mentioned here, which could help balance that.
  • Easier to mege, but with some quality constraints. I.e., [ADD] modules with full test script, can merged with 1 reviewer.
  • Close the inactivity PR very fast, less poison to reviewers
  • The [MIG] module should be easier to merge, i.e., if a full test is added no reviewer is required, or maintainer can do the merge himself.
I think we should use Test Coverage as an incentive to contributors if he/she want to get merge faster.

Kitti U.

PS: Thank you Daniel, for sharing the article. It is interesting and may fit with nature of Odoo nowadays.

On Thu, Sep 16, 2021 at 7:51 PM Sylvain LE GAL <> wrote:

Thanks for the topic.

Some response in your email.

kind regards.

Sylvain LE GAL - Twitter
GRAP - Service informatique (Groupement Régional Alimentaire de Proximité)
Site Web | FramaSphere | Facebook
3 Grande rue des Feuillants, 69001 Lyon
Standard : (+33)
Service Informatique : (+33)
Astreinte Informatique : (+33)
Member of the OCA (Odoo Community Association)

Le jeu. 16 sept. 2021 à 13:51, Tom Blauwendraat <> a écrit :
Hi all,

For years, OCA has a big problem with unmerged PR's.

That's true ! One of the origins is the fast release rate of Odoo, which requires continuous migration of modules that, for many, do not change.

Unfortunately, that situation will not change. Odoo policy seems to be : "a year = a release".

let's put it into perspective a little bit :

- maintainers is a quite "new concept" in the OCA. We should continue to promote the concept.
- PSC members should be updated. I worked recently to update PSC member for OCA/pos, here removing for exemple a guy that stopped working on odoo 5 years ago... I think it should be generalized.
- we generally organize code sprint to merge PRs, but with the covid, it's been two years since there was a "real" event (only online), and I think this has disturbed the process

Recently, we have added on the repo homepage, the maintainer icons. This allows to see quickly which modules are not maintained. The result is good.

Other recent improvment has been done in the bot and maintainer-tools project. I think that allowing maintainers to merge "[MIG]" PRs could be a great addition.
Also, even if there are 2 reviews then PSC's generally don't respond to 
merge requests.
That depends a lot of PSC and repo. some PSC of some repo are very reactive.
The answer has always been "let's review more" or "Let's use 
gitaggregator and so we can use unmerged PR's".

But why don't we try something more radical:

- Let's write a script to assign "maintainer" role for all modules to 
the person who committed the oldest/original version of it
hum. That can be a good idea, if the automation is not total. I mean, if we create a PR, and tag people saying : "The bot want to set you as maintainers for module x, y, are you agree ? please answer" --> it's ok.
If we affect automatically people, I think we'll have modules with maintainers that don't maintain modules for real. it's worse than not having maintainers.
Some modules are 10 years old, their authors may not work on odoo at all.
- If maintainer does not respond to a ping longer than 1 month he loses 
the maintainer role, which then changes to the default maintainer that 
is set for the full repo.
Oh, no ! I'm PSC of OCA/pos. I don't want to have my name set to all modules that are unmaintained. We should assume that there are unmaintained modules. That's the reality.
- Let's require only 1 positive review from now on. After that the 
maintainer can merge.

--> I don't think that having 2 reviewers is always necessary and it 
also does not prevent bugs from being merged - this happens anyway. I 
used to have high trust of merged OCA modules but after seeing some 
quite ugly bugs and incomplete work being merged I am starting to think 
that maybe the quality of the unmerged stuff is not that bad as compared 
to what is actually merged. The maintainer can prevent really bad 
changes from entering, by just closing the PR.
As said by daniel, that is handled by the current rules, depending on the maturity.

Post to:

Post to:


  • Avatar


    Sunflower IT, Tom Blauwendraat
    - 16/09/2021 13:49:25 - 0

    10 replies 10 replies