Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#6911 enhancement closed wontfix (wontfix)

Porting twisted.persisted.styles to python3

Reported by: real Owned by: real
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Branch: branches/styles-python3-6911
branch-diff, diff-cov, branch-cov, buildbot
Author: realcr, exarkun

Description

Porting styles.py to support the syntax of python2.7 and python3.3.

First fixes in the introduced patch:

  • Fixing copyreg and copy_reg issues.
  • Using NativeStringIO instead of StringIO.

Attachments (1)

persisted_styles_py3.patch (1.7 KB) - added by real 4 years ago.
Minor syntax changes for python3 porting.

Download all attachments as: .zip

Change History (13)

Changed 4 years ago by real

Attachment: persisted_styles_py3.patch added

Minor syntax changes for python3 porting.

comment:1 Changed 4 years ago by Jean-Paul Calderone

Keywords: python3 review removed
Owner: set to real

Comments for this are basically the same as for #6910.

comment:2 Changed 4 years ago by Jean-Paul Calderone

Owner: changed from real to Jean-Paul Calderone

comment:3 Changed 4 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/styles-python3-6911

(In [41336]) Branching to 'styles-python3-6911'

comment:4 Changed 4 years ago by Jean-Paul Calderone

(In [41337]) Apply persisted_styles_py3.patch

refs #6911

comment:5 Changed 4 years ago by Jean-Paul Calderone

(In [41338]) Add this module and the test module to the ported list so we can run the tests on Python 3

refs #6911

comment:6 Changed 4 years ago by Jean-Paul Calderone

(In [41339]) Split twisted.test.test_persisted into several twisted.persisted.test modules; update setup3.py to reflect that only twisted.persisted.styles is being ported right now.

refs #6911

comment:7 Changed 4 years ago by Jean-Paul Calderone

Owner: changed from Jean-Paul Calderone to real

I planned to port this completely as a demonstration of how. It seemed like a good idea since twisted.persisted.styles is almost a "leaf" module (it has almost no dependencies, and it looks like its dependencies are all ported already).

However, the pickle module seems to have changed on Python 3 in a way that makes it somewhat difficult to implement Ephemeral and very difficult to implement Versioned. Also this code is all kind of unused and should probably be removed instead of ported.

So, unfortunately, I'm giving up instead.

comment:8 Changed 4 years ago by Jean-Paul Calderone

Author: exarkunrealcr, exarkun

comment:9 Changed 4 years ago by Tom Prince

Buildbot uses Versioned, so I would be sad if it got removed. We don't care about python3, though.

comment:10 Changed 3 years ago by Jean-Paul Calderone

Resolution: wontfix
Status: newclosed

Buildbot uses Versioned, so I would be sad if it got removed.

That's a bummer. :/ What are the chances Buildbot will switch to a real persistence system, or at the very least not a pickle-based joke maintained by Twisted?

We don't care about python3, though.

I'd really like to deprecate the whole module. At the very least it seems like a total waste of time to port it to Python 3 - which probably means removing the use of it from the rest of Twisted. And once Twisted itself isn't using it, it's probably a good idea to drop it...

At worst, Buildbot could import the code?

Closing this, in any case, because the module should not be ported.

comment:11 in reply to:  9 Changed 3 years ago by Glyph

Replying to tom.prince:

Buildbot uses Versioned, so I would be sad if it got removed. We don't care about python3, though.

This comment reminded me of one of the things that has made python 3 itself such a disaster - big, breaking API changes in the middle of big, breaking language changes, because "now we have the chance to do it right". For the most part, we decided not to do this ourselves.

twisted.persisted is a gross package, but the right way to fix a gross package is to maintain it, to deprecate the gross parts, to come up with better alternatives and to go through the normal process for getting rid of it. I think that trying to lever the py3 port into a way of getting rid of these packages has thus far been a bad plan. It seems to be causing lots of re-work, stalling, and concern about compatibility every time someone hits a class that uses one of these things. And we are going to keep hitting them - for example, twistd -y - hardly an obscure feature - is still implemented with twisted.persisted.sob.

Most of all, I don't think that porting twisted.persisted is actually all that much work, but discussion around it has made it seem as though it would be overwhelming. To try to see if I was right about that, I have done a proof-of-concept over on #7827 of porting all the existing tests for twisted.persisted over. I think I hit them all. It probably needs more stuff, and I anticipate a couple of review cycles on it, but I think just powering through and getting this ported so we can deprecate it at the same time in 2 and 3 will speed up a lot of other porting efforts.

comment:12 Changed 3 years ago by Jean-Paul Calderone

twisted.persisted is a gross package, but the right way to fix a gross package is to maintain it, to deprecate the gross parts

They're all gross. None of them are not gross.

to come up with better alternatives

Plenty of better alternatives exist already. They're just not part of Twisted. Twisted doesn't need to be a database library, does it?

Most of all, I don't think that porting twisted.persisted is actually all that much work

Well, I tried it and it was more work than it seemed worth. I guess it's lucky for BuildBot that you're smart or willing to put in more work (although I think hawkowl *already* *also* figured it out so you might have been wasting your time).

Of course if you feel like doing this work, great. Certainly the decision about whether to remove this package from Twisted should be based on more interesting considerations than how hard it is to port to Python 3. Someone should now review your work and in due course it should presumably be merged (not that you need me to say that but I thought I would just to be clear that I'm not saying the opposite).

But it seems pretty clear to me that those other considerations all point towards removing it.

Also, just as a point of process, I'm not sure why this ticket is still closed if you think this module should be ported. #7827 seemed dedicated to porting Ephemeral *only* but it looks like your branch ports the entire .. package? At least the entire twisted.persisted.styles module.

Note: See TracTickets for help on using tickets.