Opened 3 years ago

Closed 3 years ago

Last modified 15 months ago

#7831 enhancement closed fixed (fixed)

twisted.python.urlpath should work on Python 3

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

Description


Attachments (2)

urlpath-python3.patch (2.1 KB) - added by _sunu_ 3 years ago.
urlpath-python3-2.patch (2.4 KB) - added by _sunu_ 3 years ago.

Download all attachments as: .zip

Change History (13)

Changed 3 years ago by _sunu_

Attachment: urlpath-python3.patch added

comment:1 Changed 3 years ago by _sunu_

Keywords: review added
Owner: set to _sunu_
Status: newassigned

The patch is based on trunk fwiw.

comment:2 Changed 3 years ago by Chris Wolfe

Owner: _sunu_ deleted
Status: assignednew

Hi _sunu_,

Thanks for your patch! I'm going to reassign this to the blank user to signal other reviewers that they can review your ticket. Otherwise, it will look like _you_ want to review the patch.

Here is a bit more of an explanation of the steps involved in submitting a patch: http://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch

Thanks again!

comment:3 Changed 3 years ago by Adi Roiban

Author: adiroiban
Branch: branches/py3-urlpath-7831

(In [44519]) Branching to py3-urlpath-7831.

comment:4 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to _sunu_

Changes looks good.

For urlpath.py

instead of

# -*- test-case-name: twisted.test.test_paths

Maybe we should have this to match the dedicated test module

# -*- test-case-name: twisted.python.test.test_urlpath -*-

I see that python3 tests pass but I think that

class URLPath:

should be

class URLPath(object):

to make sure we have same class inheritance in python2.7 and python3.


There is a linter error

************* Module twisted.python.urlpath
W9013: 18,0: Expected 3 blank lines, found 2

Please see the above comment and add your comments :).

If you are ok with the changes either let me know and I will apply them before merge, or attach a new patch.

Thanks!

Changed 3 years ago by _sunu_

Attachment: urlpath-python3-2.patch added

comment:5 Changed 3 years ago by _sunu_

Keywords: review added

Thanks for the review :)

I just added a updated patch. I didn't know about the twistedchecker tool. It's showing a bunch of other code style errors other than the one you mentioned. Should I fix those too?

comment:6 Changed 3 years ago by _sunu_

Owner: _sunu_ deleted

comment:7 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to _sunu_

Changes looks good. Test pass.

You don't have to solve the other twistedchecker errors... at this stage twistedchecker is broken :(. At this stage we only core for the new errors. Hope it will be fixed soon.

One little question before merging this: why rename unquote as unquoteFunc ? why not leave it as unquote?

Thanks again!

comment:8 Changed 3 years ago by _sunu_

Keywords: review added
Owner: _sunu_ deleted

There is already a parameter named unquote used in that function. See https://github.com/twisted/twisted/blob/trunk/twisted/python/urlpath.py#L22

comment:9 Changed 3 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [44540]) Merge py3-urlpath-7831: Port twisted.python.urlpath to Python3.

Author: _sunu_ Reviewers: adiroiban Fixes: #7831

comment:10 Changed 3 years ago by Adi Roiban

Many thanks for your work! Merged.

comment:11 Changed 15 months ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.