[Twisted-Python] Need clarification on reviews for Python 3 fixes for Twisted

[Craig Rodrigues]

Hi,

I have submitted some Python 3 patches for Twisted.

1. PATCHES REVIEWED AND COMMITTED TO TRUNK
=============================================

Use new syntax for catching exceptions
https://twistedmatrix.com/trac/ticket/8344

Use new syntax for raising exceptions
https://twistedmatrix.com/trac/ticket/8345

Remove old syntax for octal literals
https://twistedmatrix.com/trac/ticket/8347

Fix parentheses in list comprehensions
https://twistedmatrix.com/trac/ticket/8351


2. PATCHES WHICH STILL NEED TO BE REVIEWED, NOT YET COMMITTED TO TRUNK
=======================================================================

Change print to print()
http://twistedmatrix.com/trac/ticket/5812

Change foo.has_key(bar) to "bar in foo"
https://twistedmatrix.com/trac/ticket/8359
https://twistedmatrix.com/trac/ticket/8360
https://twistedmatrix.com/trac/ticket/8361
https://twistedmatrix.com/trac/ticket/8362
https://twistedmatrix.com/trac/ticket/8363
https://twistedmatrix.com/trac/ticket/8364
https://twistedmatrix.com/trac/ticket/8365

Eliminate the use of long literals
https://twistedmatrix.com/trac/ticket/8366

Remove use of tuple parameter packing
https://twistedmatrix.com/trac/ticket/8346


Adi has reviewed and committed the patches in 1.
However, Adi has mentioned that in this document:
http://twistedmatrix.com/trac/wiki/Plan/Python3,
the strategy of submitting incremental Python3 fixes is not mentioned.
Before doing any further reviews, Adi would like clarification that
these types of reviews/patches are OK for submission and review.

Are they OK? Would it be possible extend the Plan/Python3 document to
accept incremental Python3 fixes
as long as:

* adheres to Twisted coding standards
* works on Python 2.7
* passes existing tests
* comes with new tests if functionality is changed that is not currently
being tested

My experience working with Python3 on other projects, is that incremental
fixes is easier to review and get working, rather than an all or nothing
approach.
Some Python3 porting such as bytes/string/unicode or Python C API changes
are very hard,
while print vs. print() are very easy. Holding up the easy changes, until
every hard change
is also done is quite hard, and slows things down.

--
Craig

[Itamar Turner-Trauring]

Post by Craig Rodrigues
http://twistedmatrix.com/trac/wiki/Plan/Python3,
the strategy of submitting incremental Python3 fixes is not mentioned.
Before doing any further reviews, Adi would like clarification that
these types of reviews/patches are OK for submission and review.
Are they OK? Would it be possible extend the Plan/Python3 document to
accept incremental Python3 fixes
* adheres to Twisted coding standards
* works on Python 2.7
* passes existing tests
* comes with new tests if functionality is changed that is not
currently being tested
My experience working with Python3 on other projects, is that incremental
fixes is easier to review and get working, rather than an all or
nothing approach.
Some Python3 porting such as bytes/string/unicode or Python C API
changes are very hard,
while print vs. print() are very easy. Holding up the easy changes,
until every hard change
is also done is quite hard, and slows things down.

I think they're fine to accept insofar as:

1. There is strong ongoing momentum for the port now, so these changes
makes porting module-by-module easier and won't just bitrot.
2. They're doing one particular incompatibility at a time, rather than
"here's an assortment of random changes to a module that may or may not
port that module fully, who knows."

I don't think they are sufficient to port a module (someone needs to
read the code and think a bit, usually), but they will make it easier to
do so, so they definitely are worth continuing.

-Itamar

[Wolfgang Rohdewald]

Post by Itamar Turner-Trauring

Post by Craig Rodrigues
http://twistedmatrix.com/trac/wiki/Plan/Python3,
the strategy of submitting incremental Python3 fixes is not mentioned.
Before doing any further reviews, Adi would like clarification that
these types of reviews/patches are OK for submission and review.
Are they OK? Would it be possible extend the Plan/Python3 document to
accept incremental Python3 fixes
* adheres to Twisted coding standards
* works on Python 2.7
* passes existing tests
* comes with new tests if functionality is changed that is not
currently being tested
My experience working with Python3 on other projects, is that incremental
fixes is easier to review and get working, rather than an all or
nothing approach.
Some Python3 porting such as bytes/string/unicode or Python C API
changes are very hard,
while print vs. print() are very easy. Holding up the easy changes,
until every hard change
is also done is quite hard, and slows things down.

1. There is strong ongoing momentum for the port now, so these changes
makes porting module-by-module easier and won't just bitrot.
2. They're doing one particular incompatibility at a time, rather than
"here's an assortment of random changes to a module that may or may not
port that module fully, who knows."
I don't think they are sufficient to port a module (someone needs to
read the code and think a bit, usually), but they will make it easier to
do so, so they definitely are worth continuing.
-Itamar

This would have been helpful when I tried to port PB to python3.
Instead, that port is now bitrotting. I did try hard to deliver
simple changes (like print()) before tackling harder problems
but not much of all that went into the source code. Interest in
PB does not seem very high.

Anyway there still is the public git fork (I did mention it here
at that time) - if anybody would like to integrate that. Not me -
for the foreseeable future.

[Craig Rodrigues]

On Fri, May 27, 2016 at 6:14 AM, Wolfgang Rohdewald <

Post by Wolfgang Rohdewald
This would have been helpful when I tried to port PB to python3.
Instead, that port is now bitrotting.

Can you point me to your attempt to port PB to python3?
Glyph and cyli have been merging a lot of my simple python3 fixes,
so now I am motivated to do more. :)

--
Craig

[Wolfgang Rohdewald]

Post by Craig Rodrigues
On Fri, May 27, 2016 at 6:14 AM, Wolfgang Rohdewald <

Post by Wolfgang Rohdewald
This would have been helpful when I tried to port PB to python3.
Instead, that port is now bitrotting.

Can you point me to your attempt to port PB to python3?
Glyph and cyli have been merging a lot of my simple python3 fixes,
so now I am motivated to do more. :)
--
Craig

You are welcome: https://github.com/wrohdewald/twisted/commits/spread-py3-7598/twisted

[Adi Roiban]

On 27 May 2016 at 13:13, Itamar Turner-Trauring <***@itamarst.org> wrote:
[snip]

Post by Itamar Turner-Trauring
1. There is strong ongoing momentum for the port now, so these changes
makes porting module-by-module easier and won't just bitrot.

How do you define a "strong ongoing momentum" ?

Post by Itamar Turner-Trauring
2. They're doing one particular incompatibility at a time, rather than
"here's an assortment of random changes to a module that may or may not
port that module fully, who knows."

Some code parts don't have python 2.7 coverage .
Is is still acceptable to touch that code ? :)

Regards,
Adi

[Glyph]

Post by Adi Roiban
[snip]
1. There is strong ongoing momentum for the port now, so these changes makes porting module-by-module easier and won't just bitrot.
How do you define a "strong ongoing momentum" ?

I don't think "momentum" is a real thing. Investment in Twisted has historically followed an extreme boom/bust cycle, and we don't want to make any decisions assuming that work will be continuing at the current rate.

Post by Adi Roiban
2. They're doing one particular incompatibility at a time, rather than "here's an assortment of random changes to a module that may or may not port that module fully, who knows."
Some code parts don't have python 2.7 coverage .
Is is still acceptable to touch that code ? :)

No. Test coverage is how we know that the behavior is the same on both versions of Python and we're not just hoping that it is.

-glyph

[Glyph]

Post by Glyph

Post by Adi Roiban
2. They're doing one particular incompatibility at a time, rather than "here's an assortment of random changes to a module that may or may not port that module fully, who knows."
Some code parts don't have python 2.7 coverage .
Is is still acceptable to touch that code ? :)

No. Test coverage is how we know that the behavior is the same on both versions of Python and we're not just hoping that it is.

I was on a plane when I wrote this, and it may have been a bit overly terse. Let me expand a little bit.

The lines of code in a diff must be covered. However, if one is porting, let's say, a 2000-line module with no test coverage to be importable under python 3, and 8 lines of code in that module need to change, *only those 8 lines must be covered*. Obviously, the tests should be as reasonable as possible and should not do anything too evil and gross to get those lines covered, but the author of the change hitting those lines is not responsible for a ground-up comprehensive test suite design for the entire module. They just need to write the bare minimum test coverage to ensure their changes are covered at all. If you're making syntax changes, you also don't need to worry about getting those tests to actually pass on python3; it would be great to make smaller, individual changes which do the syntax first, then later actually start adding tests to the py3 suite.

So please don't take this requirement for test coverage to mean that you have to embark on a major project to land these syntax fixes. It would be great to have syntax fixes like this, and a bare minimum of very simple test coverage, just enough to exercise the lines in question, is perfectly adequate for such a small mechanical change.

-glyph

[Adi Roiban]

Post by Itamar Turner-Trauring
2. They're doing one particular incompatibility at a time, rather than

Post by Itamar Turner-Trauring
"here's an assortment of random changes to a module that may or may not
port that module fully, who knows."

Some code parts don't have python 2.7 coverage .
Is is still acceptable to touch that code ? :)
No. Test coverage is how we *know* that the behavior is the same on both
versions of Python and we're not just hoping that it is.
I was on a plane when I wrote this, and it may have been a bit overly
terse. Let me expand a little bit.
The lines of code in a diff must be covered. However, if one is porting,
let's say, a 2000-line module with no test coverage to be importable under
python 3, and 8 lines of code in that module need to change, *only those 8
lines must be covered*. Obviously, the tests should be as reasonable as
possible and should not do anything too evil and gross to get those lines
covered, but the author of the change hitting those lines is *not* responsible
for a ground-up comprehensive test suite design for the entire module.
They just need to write the bare minimum test coverage to ensure their
changes are covered at all. If you're making syntax changes, you also
don't need to worry about getting those tests to actually pass on python3;
it would be great to make smaller, individual changes which do the syntax
first, then later actually start adding tests to the py3 suite.
So please don't take this requirement for test coverage to mean that you
have to embark on a major project to land these syntax fixes. It would be
great to have syntax fixes like this, and a bare minimum of very simple
test coverage, just enough to exercise the lines in question, is perfectly
adequate for such a small mechanical change.

Thanks for the guidance.

I have updated the python 3 porting wiki page
https://twistedmatrix.com/trac/wiki/Plan/Python3#Mechanicalchanges

Here is the diff
https://twistedmatrix.com/trac/wiki/Plan/Python3?sfp_email=&sfph_mail=&action=diff&version=81&old_version=80&sfp_email=&sfph_mail=


Regards,
Adi

[Craig Rodrigues]

Post by Itamar Turner-Trauring
1. There is strong ongoing momentum for the port now, so these changes
makes porting module-by-module easier and won't just bitrot.
2. They're doing one particular incompatibility at a time, rather than
"here's an assortment of random changes to a module that may or may not
port that module fully, who knows."
I don't think they are sufficient to port a module (someone needs to read
the code and think a bit, usually), but they will make it easier to do so,
so they definitely are worth continuing.

Thanks! You have summarized exactly the strategy I have been using when
submitting these patches.
At Pycon, Glyph and cyli managed to review and merge a number of my patches.

Do you have any bandwidth to review some of the print -> print() changes I
did? They are here:

http://bit.ly/24r2fuJ

Thanks.

--
Craig

[Glyph]

Post by Craig Rodrigues
At Pycon, Glyph and cyli managed to review and merge a number of my patches.

I would like to point out that at the sprints, I was nearly continuously engaged in conversation, so I should be able to review many more now that PyCon is done :). Thanks again for submitting all of these.

-glyph