[Openerp-community] pull request reviews

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Openerp-community] pull request reviews

Leonardo Pistone - camptocamp
Hi all,

after a discussion with Pedro, Sandy and Maxime on the pull request
https://github.com/OCA/account-analytic/pull/2 , a general point came
out: I always thought that anybody can do reviews, and then someone
from the maintainer team can take the final decision to do the merge.

The name "reviewer team" is misleading: the team cannot and should not
make its code reviews alone. It just takes the final decision. I
suggest adding a note to clarify that on
https://pad.odoo.com/p/community-review .

Joël, can you please confirm if that was the idea?

On a sidenote, IMHO, while we should have rules, in cases like that I
trust maintainers to bend rules from time to time a bit like Pedro
did, (the reason  was to get the branch green ASAP, and we're only
talking about linting). That should be the exception, not the rule, of
course.

If someone else disagrees, we can make another PR fast:
https://github.com/OCA/account-analytic/pull/3 .

Thanks!

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

Pedro Manuel Baeza Romero
Hi all,

Leonardo, thanks for bringing this topic to debate. This has been said another times, that we must encourage people to review things, and I think we are in the way, but dirty jobs like the ones we are doing now (migration + PEP8) it's very difficult to get them reviewed. In last spanish OpenERP days (they were OpenERP yet ;) ), I convince a lot of people to start contributing (you can see contributions or reviews from Avanzosc or FactorLibre), but there's still a lot to do.

What Leonardo says can be a start. When we "stabilize" all the migration, translations and so on, my next step will be to publish on Odoo Spain site information in spanish on how to collaborate. I hope this serves to avoid these problems.

Regards.


2014-08-05 9:31 GMT+02:00 Leonardo Pistone <[hidden email]>:
Hi all,

after a discussion with Pedro, Sandy and Maxime on the pull request
https://github.com/OCA/account-analytic/pull/2 , a general point came
out: I always thought that anybody can do reviews, and then someone
from the maintainer team can take the final decision to do the merge.

The name "reviewer team" is misleading: the team cannot and should not
make its code reviews alone. It just takes the final decision. I
suggest adding a note to clarify that on
https://pad.odoo.com/p/community-review .

Joël, can you please confirm if that was the idea?

On a sidenote, IMHO, while we should have rules, in cases like that I
trust maintainers to bend rules from time to time a bit like Pedro
did, (the reason  was to get the branch green ASAP, and we're only
talking about linting). That should be the exception, not the rule, of
course.

If someone else disagrees, we can make another PR fast:
https://github.com/OCA/account-analytic/pull/3 .

Thanks!


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

Markus Schneider
In reply to this post by Leonardo Pistone - camptocamp
Hi,

I think review code can be the first step before contributing code. So
you need to read the guidelines and how to check it. It is annoying if
you proud to submit your first module and than realize about all the
code convention you not think about yet.
And then all people learn right from the beginning reviewing is a
important think with benefits us all. So i suggest to see the topic more
in the way, how to make the first steps more easy.

i think one problem to get into the community is that the information to
start work is spread and than it is hard which are obsolete and which
one are correctly updated.

Even: http://odoo-community.org/page/website.faq is the best way to
start and need some updates.

Hard to find: https://pad.odoo.com/p/community-review and if you are
new, you find a color page.

I think the should be a "First Steps in odoo community" page on the OCA
homepage.

This should include:

How can i start?

Just pick one topic:
-> Join your Translation Team
-> Write some Module
-> Review some Module
-> Join you Localization Team

For each there should be a detail description like:
- Where can i find more information, like:
-> List of all Projects to Contribute
-> List of all Pull-Request to Review
-> List ot all planted Todos in Localization Team

- Where can get in touch with people
-> (special) Mailing-List
-> Responsible Project People

- Where can i find a checklist
Maybe have a clear Version of https://pad.odoo.com/p/community-review to
have a easy checklist for contributer and reviewer. In two parts:
A) Overview Checklist
B) Details hints to each point of the Checklist
c) OpenERP Version specific hints (to see what is related to 6.1,7.0 and
8.0 API)

General hints to start
-> chose one topic to work on
-> stick to one version
-> read this tutorial
-> look at this example module

What are the benefits for me
-> reviewing code makes your code better
...

This is just my first outline. If we have a English version, where we
collect all important information, we can translate it to different
language.

Kind Regards

Markus



On 05.08.2014 09:31, Leonardo Pistone wrote:

> Hi all,
>
> after a discussion with Pedro, Sandy and Maxime on the pull request
> https://github.com/OCA/account-analytic/pull/2 , a general point came
> out: I always thought that anybody can do reviews, and then someone
> from the maintainer team can take the final decision to do the merge.
>
> The name "reviewer team" is misleading: the team cannot and should not
> make its code reviews alone. It just takes the final decision. I
> suggest adding a note to clarify that on
> https://pad.odoo.com/p/community-review .
>
> Joël, can you please confirm if that was the idea?
>
> On a sidenote, IMHO, while we should have rules, in cases like that I
> trust maintainers to bend rules from time to time a bit like Pedro
> did, (the reason  was to get the branch green ASAP, and we're only
> talking about linting). That should be the exception, not the rule, of
> course.
>
> If someone else disagrees, we can make another PR fast:
> https://github.com/OCA/account-analytic/pull/3 .
>
> Thanks!
>
> _______________________________________________
> Mailing list: https://launchpad.net/~openerp-community
> Post to     : [hidden email]
> Unsubscribe : https://launchpad.net/~openerp-community
> More help   : https://help.launchpad.net/ListHelp
>

--
Dipl.-Comp.-Math. Markus Schneider
Softwareentwickler

initOS GmbH & Co. KG
An der Eisenbahn 1
21224 Rosengarten

Mobil:   +49 (0)172 2303699
Phone:   +49 (0)4105 5615613
Fax:     +49 (0)4105 5615610

Email:   [hidden email]
Web:     http://www.initos.com

Geschäftsführung:
Dipl. Wirt.-Inf. Frederik Kramer & Dipl.-Ing. (FH) Torsten Francke
Haftende Gesellschafterin: initOS Verwaltungs GmbH

Sitz der Gesellschaft: Rosengarten – Klecken
Amtsgericht Tostedt, HRA 201840
USt-IdNr: DE 275698169
Steuer-Nr: 15/205/21402

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

Joël Grand-Guillaume @ camptocamp


On Tue, Aug 5, 2014 at 11:41 AM, Markus Schneider <[hidden email]> wrote:
Hi,

I think review code can be the first step before contributing code. So
you need to read the guidelines and how to check it. It is annoying if
you proud to submit your first module and than realize about all the
code convention you not think about yet.
And then all people learn right from the beginning reviewing is a
important think with benefits us all. So i suggest to see the topic more
in the way, how to make the first steps more easy.

i think one problem to get into the community is that the information to
start work is spread and than it is hard which are obsolete and which
one are correctly updated.

Even: http://odoo-community.org/page/website.faq is the best way to
start and need some updates.

Hard to find: https://pad.odoo.com/p/community-review and if you are
new, you find a color page.

I think the should be a "First Steps in odoo community" page on the OCA
homepage.

This should include:

How can i start?

Just pick one topic:
-> Join your Translation Team
-> Write some Module
-> Review some Module
-> Join you Localization Team

For each there should be a detail description like:
- Where can i find more information, like:
-> List of all Projects to Contribute
-> List of all Pull-Request to Review
-> List ot all planted Todos in Localization Team

- Where can get in touch with people
-> (special) Mailing-List
-> Responsible Project People

- Where can i find a checklist
Maybe have a clear Version of https://pad.odoo.com/p/community-review to
have a easy checklist for contributer and reviewer. In two parts:
A) Overview Checklist
B) Details hints to each point of the Checklist
c) OpenERP Version specific hints (to see what is related to 6.1,7.0 and
8.0 API)

General hints to start
-> chose one topic to work on
-> stick to one version
-> read this tutorial
-> look at this example module

What are the benefits for me
-> reviewing code makes your code better
...

This is just my first outline. If we have a English version, where we
collect all important information, we can translate it to different
language.

Kind Regards

Markus



On 05.08.2014 09:31, Leonardo Pistone wrote:
> Hi all,
>
> after a discussion with Pedro, Sandy and Maxime on the pull request
> https://github.com/OCA/account-analytic/pull/2 , a general point came
> out: I always thought that anybody can do reviews, and then someone
> from the maintainer team can take the final decision to do the merge.
>
> The name "reviewer team" is misleading: the team cannot and should not
> make its code reviews alone. It just takes the final decision. I
> suggest adding a note to clarify that on
> https://pad.odoo.com/p/community-review .
>
> Joël, can you please confirm if that was the idea?
>
> On a sidenote, IMHO, while we should have rules, in cases like that I
> trust maintainers to bend rules from time to time a bit like Pedro
> did, (the reason  was to get the branch green ASAP, and we're only
> talking about linting). That should be the exception, not the rule, of
> course.
>
> If someone else disagrees, we can make another PR fast:
> https://github.com/OCA/account-analytic/pull/3 .
>
> Thanks!
>
> _______________________________________________
> Mailing list: https://launchpad.net/~openerp-community
> Post to     : [hidden email]
> Unsubscribe : https://launchpad.net/~openerp-community
> More help   : https://help.launchpad.net/ListHelp
>

--
Dipl.-Comp.-Math. Markus Schneider
Softwareentwickler

initOS GmbH & Co. KG
An der Eisenbahn 1
21224 Rosengarten

Mobil:   <a href="tel:%2B49%20%280%29172%202303699" value="+491722303699">+49 (0)172 2303699
Phone:   <a href="tel:%2B49%20%280%294105%205615613" value="+4941055615613">+49 (0)4105 5615613
Fax:     <a href="tel:%2B49%20%280%294105%205615610" value="+4941055615610">+49 (0)4105 5615610

Email:   [hidden email]
Web:     http://www.initos.com

Geschäftsführung:
Dipl. Wirt.-Inf. Frederik Kramer & Dipl.-Ing. (FH) Torsten Francke
Haftende Gesellschafterin: initOS Verwaltungs GmbH

Sitz der Gesellschaft: Rosengarten – Klecken
Amtsgericht Tostedt, HRA 201840
USt-IdNr: DE 275698169
Steuer-Nr: 15/205/21402



--


camptocamp
INNOVATIVE SOLUTIONS
BY OPEN SOURCE EXPERTS

Joël Grand-Guillaume
Division Manager
Business Solutions

+41 21 619 10 28



_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

Pedro Manuel Baeza Romero
I agree that we need to expand documentation about how to contribute. As I said, I want to do it for spanish community as soon as we finish the rest of the tasks.

Regards.


2014-08-05 11:45 GMT+02:00 Joël Grand-Guillaume <[hidden email]>:


On Tue, Aug 5, 2014 at 11:41 AM, Markus Schneider <[hidden email]> wrote:
Hi,

I think review code can be the first step before contributing code. So
you need to read the guidelines and how to check it. It is annoying if
you proud to submit your first module and than realize about all the
code convention you not think about yet.
And then all people learn right from the beginning reviewing is a
important think with benefits us all. So i suggest to see the topic more
in the way, how to make the first steps more easy.

i think one problem to get into the community is that the information to
start work is spread and than it is hard which are obsolete and which
one are correctly updated.

Even: http://odoo-community.org/page/website.faq is the best way to
start and need some updates.

Hard to find: https://pad.odoo.com/p/community-review and if you are
new, you find a color page.

I think the should be a "First Steps in odoo community" page on the OCA
homepage.

This should include:

How can i start?

Just pick one topic:
-> Join your Translation Team
-> Write some Module
-> Review some Module
-> Join you Localization Team

For each there should be a detail description like:
- Where can i find more information, like:
-> List of all Projects to Contribute
-> List of all Pull-Request to Review
-> List ot all planted Todos in Localization Team

- Where can get in touch with people
-> (special) Mailing-List
-> Responsible Project People

- Where can i find a checklist
Maybe have a clear Version of https://pad.odoo.com/p/community-review to
have a easy checklist for contributer and reviewer. In two parts:
A) Overview Checklist
B) Details hints to each point of the Checklist
c) OpenERP Version specific hints (to see what is related to 6.1,7.0 and
8.0 API)

General hints to start
-> chose one topic to work on
-> stick to one version
-> read this tutorial
-> look at this example module

What are the benefits for me
-> reviewing code makes your code better
...

This is just my first outline. If we have a English version, where we
collect all important information, we can translate it to different
language.

Kind Regards

Markus



On 05.08.2014 09:31, Leonardo Pistone wrote:
> Hi all,
>
> after a discussion with Pedro, Sandy and Maxime on the pull request
> https://github.com/OCA/account-analytic/pull/2 , a general point came
> out: I always thought that anybody can do reviews, and then someone
> from the maintainer team can take the final decision to do the merge.
>
> The name "reviewer team" is misleading: the team cannot and should not
> make its code reviews alone. It just takes the final decision. I
> suggest adding a note to clarify that on
> https://pad.odoo.com/p/community-review .
>
> Joël, can you please confirm if that was the idea?
>
> On a sidenote, IMHO, while we should have rules, in cases like that I
> trust maintainers to bend rules from time to time a bit like Pedro
> did, (the reason  was to get the branch green ASAP, and we're only
> talking about linting). That should be the exception, not the rule, of
> course.
>
> If someone else disagrees, we can make another PR fast:
> https://github.com/OCA/account-analytic/pull/3 .
>
> Thanks!
>
> _______________________________________________
> Mailing list: https://launchpad.net/~openerp-community
> Post to     : [hidden email]
> Unsubscribe : https://launchpad.net/~openerp-community
> More help   : https://help.launchpad.net/ListHelp
>

--
Dipl.-Comp.-Math. Markus Schneider
Softwareentwickler

initOS GmbH & Co. KG
An der Eisenbahn 1
21224 Rosengarten

Mobil:   <a href="tel:%2B49%20%280%29172%202303699" value="+491722303699" target="_blank">+49 (0)172 2303699
Phone:   <a href="tel:%2B49%20%280%294105%205615613" value="+4941055615613" target="_blank">+49 (0)4105 5615613
Fax:     <a href="tel:%2B49%20%280%294105%205615610" value="+4941055615610" target="_blank">+49 (0)4105 5615610

Email:   [hidden email]
Web:     http://www.initos.com

Geschäftsführung:
Dipl. Wirt.-Inf. Frederik Kramer & Dipl.-Ing. (FH) Torsten Francke
Haftende Gesellschafterin: initOS Verwaltungs GmbH

Sitz der Gesellschaft: Rosengarten – Klecken
Amtsgericht Tostedt, HRA 201840
USt-IdNr: DE 275698169
Steuer-Nr: 15/205/21402



--


camptocamp
INNOVATIVE SOLUTIONS
BY OPEN SOURCE EXPERTS

Joël Grand-Guillaume
Division Manager
Business Solutions

+41 21 619 10 28




_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

eLBatti
In reply to this post by Leonardo Pistone - camptocamp
On 08/05/2014 09:31 AM, Leonardo Pistone wrote:
> Hi all,
>
> after a discussion with Pedro, Sandy and Maxime on the pull request
> https://github.com/OCA/account-analytic/pull/2 , a general point came
> out: I always thought that anybody can do reviews, and then someone
> from the maintainer team can take the final decision to do the merge.

I was assuming that too.


--
Lorenzo Battistini
Tel (CH): +41 91 210 23 40
Tel (IT): +39 011 198 25481
http://www.agilebg.com
https://github.com/eLBati


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

Sandy Carter (http://www.savoirfairelinux.com)
Just for the record, I don't object to you reviewing. I should like to
encourage you to.

Le 2014-08-06 04:07, Lorenzo Battistini a écrit :

> On 08/05/2014 09:31 AM, Leonardo Pistone wrote:
>> Hi all,
>>
>> after a discussion with Pedro, Sandy and Maxime on the pull request
>> https://github.com/OCA/account-analytic/pull/2 , a general point came
>> out: I always thought that anybody can do reviews, and then someone
>> from the maintainer team can take the final decision to do the merge.
>
> I was assuming that too.
>
>

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp

signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

Maxime Chambreuil (http://www.savoirfairelinux.com)
Leonardo,

You are right. Anybody can do reviews, but time should be given to do so.

That's why we setup some rules to wait for 5 calendar days. The rules does also mention that for extraordinary moments, a pull request can be merged in less than 5 days if approved by 3 reviewers.

We are talking about a pull request merged the same day after 2 reviews here and introducing code bypassing the PEP8 compliance check. I am sure even Pedro will agree that it does not allow too much time for others to review.

Regards,
--
Maxime Chambreuil
+1 (514) 276-5468 #126

----- Mail original -----
Just for the record, I don't object to you reviewing. I should like to
encourage you to.

Le 2014-08-06 04:07, Lorenzo Battistini a écrit :

> On 08/05/2014 09:31 AM, Leonardo Pistone wrote:
>> Hi all,
>>
>> after a discussion with Pedro, Sandy and Maxime on the pull request
>> https://github.com/OCA/account-analytic/pull/2 , a general point came
>> out: I always thought that anybody can do reviews, and then someone
>> from the maintainer team can take the final decision to do the merge.
>
> I was assuming that too.
>
>


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

Pedro Manuel Baeza Romero
That was because the next step I wanted to do (fix PEP8) was inmediately after (the same night). Sincerely, I didn't expect nobody review it so late, and I need it that because of Travis results for the next PR, that it was not going to be tested if we didn't merge that PR. But as I have said, that change was not critical, so I merge it with brain.

Regards.


2014-08-06 17:57 GMT+02:00 Maxime Chambreuil <[hidden email]>:
Leonardo,

You are right. Anybody can do reviews, but time should be given to do so.

That's why we setup some rules to wait for 5 calendar days. The rules does also mention that for extraordinary moments, a pull request can be merged in less than 5 days if approved by 3 reviewers.

We are talking about a pull request merged the same day after 2 reviews here and introducing code bypassing the PEP8 compliance check. I am sure even Pedro will agree that it does not allow too much time for others to review.

Regards,
--
Maxime Chambreuil
<a href="tel:%2B1%20%28514%29%20276-5468%20%23126" value="+15142765468">+1 (514) 276-5468 #126

----- Mail original -----
Just for the record, I don't object to you reviewing. I should like to
encourage you to.

Le 2014-08-06 04:07, Lorenzo Battistini a écrit :
> On 08/05/2014 09:31 AM, Leonardo Pistone wrote:
>> Hi all,
>>
>> after a discussion with Pedro, Sandy and Maxime on the pull request
>> https://github.com/OCA/account-analytic/pull/2 , a general point came
>> out: I always thought that anybody can do reviews, and then someone
>> from the maintainer team can take the final decision to do the merge.
>
> I was assuming that too.
>
>


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

Maxime Chambreuil (http://www.savoirfairelinux.com)
Pedro,

We are based in Montreal, Canada and could have reviewed it. It's day time for us when it's night for you. Like anyone, you don't see much when it's late at night ;)

Ping us next time and we could be your third review.

Thanks.
--
Maxime Chambreuil
+1 (514) 276-5468 #126


That was because the next step I wanted to do (fix PEP8) was inmediately after (the same night). Sincerely, I didn't expect nobody review it so late, and I need it that because of Travis results for the next PR, that it was not going to be tested if we didn't merge that PR. But as I have said, that change was not critical, so I merge it with brain.

Regards.


2014-08-06 17:57 GMT+02:00 Maxime Chambreuil <[hidden email]>:
Leonardo,

You are right. Anybody can do reviews, but time should be given to do so.

That's why we setup some rules to wait for 5 calendar days. The rules does also mention that for extraordinary moments, a pull request can be merged in less than 5 days if approved by 3 reviewers.

We are talking about a pull request merged the same day after 2 reviews here and introducing code bypassing the PEP8 compliance check. I am sure even Pedro will agree that it does not allow too much time for others to review.

Regards,
--
Maxime Chambreuil
<a href="tel:%2B1%20%28514%29%20276-5468%20%23126" target="_blank">+1 (514) 276-5468 #126

----- Mail original -----
Just for the record, I don't object to you reviewing. I should like to
encourage you to.

Le 2014-08-06 04:07, Lorenzo Battistini a écrit :
> On 08/05/2014 09:31 AM, Leonardo Pistone wrote:
>> Hi all,
>>
>> after a discussion with Pedro, Sandy and Maxime on the pull request
>> https://github.com/OCA/account-analytic/pull/2 , a general point came
>> out: I always thought that anybody can do reviews, and then someone
>> from the maintainer team can take the final decision to do the merge.
>
> I was assuming that too.
>
>


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp



_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Openerp-community] pull request reviews

Pedro Manuel Baeza Romero
Good to know it. Sometimes, I lose the globality of the community ;)

Regards.


2014-08-06 18:24 GMT+02:00 Maxime Chambreuil <[hidden email]>:
Pedro,

We are based in Montreal, Canada and could have reviewed it. It's day time for us when it's night for you. Like anyone, you don't see much when it's late at night ;)

Ping us next time and we could be your third review.

Thanks.

--
Maxime Chambreuil
<a href="tel:%2B1%20%28514%29%20276-5468%20%23126" value="+15142765468" target="_blank">+1 (514) 276-5468 #126


That was because the next step I wanted to do (fix PEP8) was inmediately after (the same night). Sincerely, I didn't expect nobody review it so late, and I need it that because of Travis results for the next PR, that it was not going to be tested if we didn't merge that PR. But as I have said, that change was not critical, so I merge it with brain.

Regards.


2014-08-06 17:57 GMT+02:00 Maxime Chambreuil <[hidden email]>:
Leonardo,

You are right. Anybody can do reviews, but time should be given to do so.

That's why we setup some rules to wait for 5 calendar days. The rules does also mention that for extraordinary moments, a pull request can be merged in less than 5 days if approved by 3 reviewers.

We are talking about a pull request merged the same day after 2 reviews here and introducing code bypassing the PEP8 compliance check. I am sure even Pedro will agree that it does not allow too much time for others to review.

Regards,
--
Maxime Chambreuil
<a href="tel:%2B1%20%28514%29%20276-5468%20%23126" target="_blank">+1 (514) 276-5468 #126

----- Mail original -----
Just for the record, I don't object to you reviewing. I should like to
encourage you to.

Le 2014-08-06 04:07, Lorenzo Battistini a écrit :
> On 08/05/2014 09:31 AM, Leonardo Pistone wrote:
>> Hi all,
>>
>> after a discussion with Pedro, Sandy and Maxime on the pull request
>> https://github.com/OCA/account-analytic/pull/2 , a general point came
>> out: I always thought that anybody can do reviews, and then someone
>> from the maintainer team can take the final decision to do the merge.
>
> I was assuming that too.
>
>


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp




_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Loading...