Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#7804 enhancement closed fixed (fixed)

Port twisted.python.modules on py3 for trial

Reported by: Adi Roiban Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Branch: branches/tpmodules-py3-7804-3
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, adiroiban

Description

As a start my plan is to port t.p.modules just that I can bootstrap trial.

I have checked the current code and the only problem is children.sort()

I already have a branch with module ported and all tests passing and will create a branch for this ticket.

Change History (23)

comment:1 Changed 7 years ago by Adi Roiban

Author: adiroiban
Branch: branches/py3-t.p.modules-7804

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

comment:2 Changed 7 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

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

Thanks!

comment:3 Changed 7 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

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.

comment:4 Changed 7 years ago by Julian Berman

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

comment:5 Changed 7 years ago by Glyph

Good catch, Julian. Thanks!

comment:6 Changed 7 years ago by Adi Roiban

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

comment:7 Changed 7 years ago by hawkowl

Author: adiroibanhawkowl, adiroiban
Branch: branches/py3-t.p.modules-7804branches/tpmodules-py3-7804

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

comment:8 Changed 7 years ago by hawkowl

Branch: branches/tpmodules-py3-7804branches/tpmodules-py3-7804-2

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

comment:9 Changed 7 years ago by hawkowl

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

comment:10 Changed 7 years ago by Adi Roiban

Ok. I guess that this is the current diff https://github.com/twisted/twisted/compare/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

comment:11 Changed 7 years ago by hawkowl

Owner: changed from Adi Roiban to hawkowl

Hi Adi,

I'll take this one over. :)

comment:12 Changed 7 years ago by hawkowl

I'm happy with this one so far.

Can be reviewed when #7805 lands.

comment:13 Changed 7 years ago by hawkowl

Branch: branches/tpmodules-py3-7804-2branches/tpmodules-py3-7804-3

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

comment:14 Changed 7 years ago by hawkowl

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)

comment:15 Changed 7 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

comment:16 Changed 7 years ago by Adi Roiban

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.
  1. do we need to bother about __all__ and __all3__ in test code?
  1. 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.

comment:17 Changed 7 years ago by hawkowl

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.

Last edited 7 years ago by hawkowl (previous) (diff)

comment:18 Changed 6 years ago by Adi Roiban

Keywords: review removed
Owner: set 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!

comment:19 Changed 6 years ago by hawkowl

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.

comment:20 Changed 6 years ago by hawkowl

Resolution: fixed
Status: newclosed

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

Author: hawkowl Reviewer: adiroiban Fixes: #7804

comment:21 Changed 6 years ago by Glyph

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

comment:22 Changed 6 years ago by hawkowl

I think so. Maybe?

comment:23 Changed 6 years ago by Adi Roiban

I have created this github issue to trac this problem https://github.com/twisted-infra/braid/issues/80

Note: See TracTickets for help on using tickets.