Re: [Python-Dev] Code reviews

2012-01-02 Thread Barry Warsaw
On Jan 02, 2012, at 06:35 PM, Georg Brandl wrote:

>Exactly. Especially for reviews of patches from non-core people, we
>should exercise a lot of restraint: as the committers, I think we can be
>expected to bite the sour bullet and apply our uniform style (such as
>it is).
>
>It is tiresome, if not downright disappointing, to get reviews that
>are basically "nothing wrong, but please submit again with one more
>empty line between the classes", and definitely not the way to
>attract more contributors.

I think it's fine in a code review to point out where the submission misses
the important consistency points, but not to hold up merging the changes
because of that.  You want to educate and motivate so that the next submission
comes closer to our standards.  The core dev who commits the change can clean
up style issues.

-Barry

P.S. +1 for the change to PEP 7.
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: 
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com


Re: [Python-Dev] Code reviews

2012-01-02 Thread Francisco Martin Brugue

On 01/02/2012 10:02 PM, julien tayon wrote:

@francis
Like indent ?
http://www.linuxmanpages.com/man1/indent.1.php

Thank you, I wasn't aware of this one !
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: 
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com


Re: [Python-Dev] Code reviews

2012-01-02 Thread julien tayon
@francis
Like indent ?
http://www.linuxmanpages.com/man1/indent.1.php

@brian
> I don't think this is a problem to the point that it needs to be fixed
> via automation. The code I write is the code I build and test, so I'd
> rather not have some script that goes in and modifies it to some
> accepted format, then have to go through the build/test dance again.


Well, it breaks committing since it adds non significative symbols,
therefore bloats the diffs.
But as far as I am concerned for using it a long time ago, it did not
break anything, it was pretty reliable.

my 2c * 0.1
-- 
jul
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: 
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com


Re: [Python-Dev] Code reviews

2012-01-02 Thread Brian Curtin
On Mon, Jan 2, 2012 at 12:26, francis  wrote:
> On 01/02/2012 06:35 PM, Georg Brandl wrote:
>>
>> On 01/02/2012 03:41 PM, Antoine Pitrou wrote:
>>>
>>> On Mon, 2 Jan 2012 14:44:49 +1000
>>> Nick Coghlan  wrote:

 He keeps leaving them out, I occasionally tell him they should always
 be included (most recently this came up when we gave conflicting
 advice to a patch contributor).
>>>
>>> Oh, by the way, this is also why I avoid arguing too much about style
>>> in code reviews. There are two bad things which can happen:
>>>
>>> - your advice conflicts with advice given by another reviewer (perhaps
>>>   on another issue)
>>> - the contributor feels drowned under tiresome requests for style
>>>   fixes ("please indent continuation lines this way")
>>>
>>> Both are potentially demotivating. A contributor can have his/her own
>>> style if it doesn't adversely affect code quality.
>>
>> Exactly. Especially for reviews of patches from non-core people, we
>> should exercise a lot of restraint: as the committers, I think we can be
>> expected to bite the sour bullet and apply our uniform style (such as
>> it is).
>>
>> It is tiresome, if not downright disappointing, to get reviews that
>> are basically "nothing wrong, but please submit again with one more
>> empty line between the classes", and definitely not the way to
>> attract more contributors.
>>
> Hi to all member of this list,
> I'm not a Python-Dev (only some very small patches over core-mentorship
> list.
> Just my 2cents here).
>
> I would try to relax this conflicts with a script that does the reformatting
> itself. If
> that reformatting where part of the process itself do you thing that that
> would
> be an issue anymore?

I don't think this is a problem to the point that it needs to be fixed
via automation. The code I write is the code I build and test, so I'd
rather not have some script that goes in and modifies it to some
accepted format, then have to go through the build/test dance again.
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: 
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com


Re: [Python-Dev] Code reviews

2012-01-02 Thread francis

On 01/02/2012 06:35 PM, Georg Brandl wrote:

On 01/02/2012 03:41 PM, Antoine Pitrou wrote:

On Mon, 2 Jan 2012 14:44:49 +1000
Nick Coghlan  wrote:

He keeps leaving them out, I occasionally tell him they should always
be included (most recently this came up when we gave conflicting
advice to a patch contributor).

Oh, by the way, this is also why I avoid arguing too much about style
in code reviews. There are two bad things which can happen:

- your advice conflicts with advice given by another reviewer (perhaps
   on another issue)
- the contributor feels drowned under tiresome requests for style
   fixes ("please indent continuation lines this way")

Both are potentially demotivating. A contributor can have his/her own
style if it doesn't adversely affect code quality.

Exactly. Especially for reviews of patches from non-core people, we
should exercise a lot of restraint: as the committers, I think we can be
expected to bite the sour bullet and apply our uniform style (such as
it is).

It is tiresome, if not downright disappointing, to get reviews that
are basically "nothing wrong, but please submit again with one more
empty line between the classes", and definitely not the way to
attract more contributors.


Hi to all member of this list,
I'm not a Python-Dev (only some very small patches over core-mentorship 
list.

Just my 2cents here).

I would try to relax this conflicts with a script that does the 
reformatting itself. If
that reformatting where part of the process itself do you thing that 
that would

be an issue anymore?

PS: I know that there’s a pep8 checker so it could be transformed into a 
reformatter

but I don't know if theres a pep7 checker (reformater)


Best regards!

francis


___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: 
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com


Re: [Python-Dev] Code reviews

2012-01-02 Thread Georg Brandl
On 01/02/2012 03:41 PM, Antoine Pitrou wrote:
> On Mon, 2 Jan 2012 14:44:49 +1000
> Nick Coghlan  wrote:
>> 
>> He keeps leaving them out, I occasionally tell him they should always
>> be included (most recently this came up when we gave conflicting
>> advice to a patch contributor).
> 
> Oh, by the way, this is also why I avoid arguing too much about style
> in code reviews. There are two bad things which can happen:
> 
> - your advice conflicts with advice given by another reviewer (perhaps
>   on another issue)
> - the contributor feels drowned under tiresome requests for style
>   fixes ("please indent continuation lines this way")
> 
> Both are potentially demotivating. A contributor can have his/her own
> style if it doesn't adversely affect code quality.

Exactly. Especially for reviews of patches from non-core people, we
should exercise a lot of restraint: as the committers, I think we can be
expected to bite the sour bullet and apply our uniform style (such as
it is).

It is tiresome, if not downright disappointing, to get reviews that
are basically "nothing wrong, but please submit again with one more
empty line between the classes", and definitely not the way to
attract more contributors.

Georg

___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: 
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com


[Python-Dev] Code reviews

2012-01-02 Thread Antoine Pitrou
On Mon, 2 Jan 2012 14:44:49 +1000
Nick Coghlan  wrote:
> 
> He keeps leaving them out, I occasionally tell him they should always
> be included (most recently this came up when we gave conflicting
> advice to a patch contributor).

Oh, by the way, this is also why I avoid arguing too much about style
in code reviews. There are two bad things which can happen:

- your advice conflicts with advice given by another reviewer (perhaps
  on another issue)
- the contributor feels drowned under tiresome requests for style
  fixes ("please indent continuation lines this way")

Both are potentially demotivating. A contributor can have his/her own
style if it doesn't adversely affect code quality.

Regards

Antoine.


___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: 
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com