@adiroiban commented

(In [43989]) Branching to py3-t.p.modules-7804.

	@adiroiban removed owner

there are still failing tests but I am just putting this for review to get initial feedback.

Thanks!

	@glyph set owner to @adiroiban

Thanks for working on this.

1. the tests are failing, as you've noticed, that needs to be fixed :)
2. the big pile of inequality methods should probably be on a helper that implements inequality; this would be less code, and also more generally useful. I'd say functools.total_ordering but that's 2.7+ only, and we still support 2.6...

Otherwise, looks basically sensible.

	@Julian commented

Even 2.6 has next(), is there something I'm missing on why it was added to t.p.compat?

	@glyph commented

Good catch, Julian. Thanks!

	@adiroiban commented

Thanks for the review.

I have removed next from compat. I was over exicted with having Trial working on Python and forgot to check if next is available in python2.6 ... I have never touched 2.6.
I hate using run-python3-tests for doing TDD

I have pushed my current changes but they are junk...and they also touch zippath.

my changes in filepath were wrong as I see that FilePath requires path to be pass as bytes... this is a PITA and I think that before continuing with Python3 port we need an Unicode friendly FilePath.

Will wait for progress from #7805

	@hawkowl commented

(In [44200]) Branching to tpmodules-py3-7804.

	@hawkowl commented

(In [44201]) Branching to tpmodules-py3-7804-2.

	@hawkowl commented

I have a working version in tpmodules-py3-7804-2. Once #7805 lands, I can put this up for review.

	@adiroiban commented

Ok. I guess that this is the current diff d5fb42b...tpmodules-py3-7804-2

As commented before I was also waiting for #7805 to land before continue working on it.

If you want to continue working in this, please feel free to change the owner.

Cheers

	@hawkowl set owner to @hawkowl

Hi Adi,

I'll take this one over. :)

	@hawkowl commented

I'm happy with this one so far.

Can be reviewed when #7805 lands.

	@hawkowl commented

(In [44246]) Branching to tpmodules-py3-7804-3.

	@hawkowl commented

Merged forward, fixed up some small things, please review.

(As an aside, this work has been done as a part of my work on Crossbar.io for Tavendo GmbH)

	@adiroiban commented

Thanks for working on this. They look good.
Twistedchecker is red.

1. Please check latest docs from trunk regarding how to add references to tickets in comments.

2. do we need to bother about __all__ and __all3__ in test code?

3. Can we just keep _ZipMapImpl in py3 but just don't register it? it could reduce the diff. Same for ZipArchive , maybe we can update it just to compile on py3 and do the actual port / tests later. The only reason for doing this is to make sure that when someone changes the _ZipMapImpl code in the future it will at least make it compile on python3

As I am just a junior reviewer I will leave this ticket on review queue so that others can review it.

	@hawkowl commented

Hi Adi! Thanks for the review.

• Fixed up those comments.
• The __all__ and __all3__ del method is recommended (instead of skips) in the Python3 plan doc.
• If we register it without actually implementing it, it's more likely to break. When zippath is ported, the support can be enabled in future.

Respinning the buildbots. I'll look into the twistedchecker failure.

	@adiroiban set owner to @hawkowl

OK.

My point with defining _ZipMapImpl in Python3 is that while working on py2.7 specific changes a developers might avoid introducing changes which are not py3 compatible.

Still some twistedchecker errors... and this just made my review 30 seconds longer as I had to check the builder and then report the errors... all this could be avoid if developers would be able to run twistedchecker on their systems

************* Module twisted
W9208: 11,0:_checkRequirements: Missing docstring
W9402: 58,0: The first letter of comment should be capitalized

Please fix the errors and merge.

Thanks!

	@hawkowl commented

Hi Adi,

Thanks!

Those twistedchecker errors are spurious -- since there's not a "new" entry, it means there's no new errors. It's from the change introduced that sets the exit flag correctly.

Will merge.

	@hawkowl set status to closed

(In [44338]) Merge tpmodules-py3-7804-3: Port twisted.python.modules to Python3

Author: hawkowl
Reviewer: adiroiban
Fixes: #7804

	@glyph commented

So we should update the buildbot config to explicitly succeed except when there are "new" errors now?

	@hawkowl commented

I think so. Maybe?

	@adiroiban commented

I have created this github issue to trac this problem twisted-infra/braid#80