Opened 4 years ago

Closed 4 years ago

#6025 enhancement closed fixed (fixed)

Port twisted.protocols.basic to Python 3

Reported by: itamar Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Branch: branches/basic-py3-6025-3
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst


twisted.protocols.basic should run on Python 3.

Change History (7)

comment:1 Changed 4 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/basic-py3-6025

(In [35830]) Branching to 'basic-py3-6025'

comment:2 Changed 4 years ago by itamar

  • Branch changed from branches/basic-py3-6025 to branches/basic-py3-6025-2
  • Keywords review added
  • Owner set to exarkun

Ugh, for some reason the branch wasn't updated. Build running here:

comment:3 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

Thanks. Looks like you had some fun on this one.

Ugh, for some reason the branch wasn't updated. Build running here:

When this happens, please follow the directions.

  1. The test-case-name on twisted/protocols/ needs to be updated
  2. The byte literals in the documentation for NetstringReceiver._LENGTH should be in C{}
  3. Lots of tests in the newly created twisted.protocols.test.test_basic don't follow the coding standard (misnamed, poorly written docstrings, use fail variation of assertion methods, etc). Please file a ticket for fixing them.
  4. I guess my general thoughts regarding _PY is that it should be used at top-level module scope and not anywhere else. Particularly the cases which aren't going to go away, like cases that deal with the differences between Python 2 str and Python 3 bytes, as in LineOnlyReceiverTestCase.test_buffer. Using it to define two alternate implementations of a helper function seems tolerable (and a function for getting an iterator over bytes that yields each individual byte as a bytes object instead of an integer sounds like one that will be used more than once anyway - new twisted.python.compat API?), but constantly re-checking it after import time bothers me, both for performance reasons and because of the tedium it introduces for debugging and overall cognitive load of a section of code. This approach would avoid worsening the Python 2 performance of the implementation of sendString, too.
    1. Other tests in this module that could use such a helper:
      1. LPTestCaseMixin.test_illegal
      2. NetstringReceiverTestCase.test_receiveNetstringPortions_3
      3. IntNTestCaseMixin.test_receive
      4. IntNTestCaseMixin.test_partial
  5. Something about new-style classes... NewStyleInt16TestCase is redundant on Python 3 I guess. Can we indicate that somehow, so eventually it's clear we can delete it?
  6. Sorry, I think the new comments are largely inappropriate - at least, as comments in the source. They would probably make a good labs blog post though.

comment:4 Changed 4 years ago by itamarst

  • Branch changed from branches/basic-py3-6025-2 to branches/basic-py3-6025-3

(In [35916]) Branching to 'basic-py3-6025-3'

comment:5 Changed 4 years ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun

Addressed all review comments. I skipped the newstyle tests, added utility method for netstring formatting, and sanitized the comment about Python 3.


comment:6 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar
  1. I guess there should be a .removal news fragment, for the removal of writing non-str.
  2. LPTestCaseMixin.test_illegal still has a subjective comment about Python 3, and some code that would be simplified by iterbytes.

That's all. Please merge once those are addressed.

comment:7 Changed 4 years ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(In [35925]) Merge basic-py3-6025-3.

Author: itamar Review: exarkun Fixes: #6025

twisted.protocols.basic now works on Python 3. Also you can't send arbitrary objects using NetstringReceiver.sendString (deprecated since v10.0).

Note: See TracTickets for help on using tickets.