Opened 11 years ago

Last modified 11 years ago

#4525 task new

Factor duplicate serial.Serial initialization out of Windows and POSIX serial port modules

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Lucas Taylor Branch: branches/refactor-duplicate-serialport-4525
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

twisted/internet/_posixserialport.py and twisted/internet/_win32serialport.py both have a BaseSerialPort subclass. Each subclass creates a serial.Serial instance in basically the same way. This isn't a lot of duplicate code, but it's a little. The main argument in favor of removing the duplication is that if a single method were responsible for this, there would be an obvious place to put documentation for the parameters.

Attachments (1)

4525-1.diff (5.4 KB) - added by Lucas Taylor 11 years ago.
Minor modifications

Download all attachments as: .zip

Change History (8)

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

Author: exarkun
Branch: branches/refactor-duplicate-serialport-4525

(In [29429]) Branching to 'refactor-duplicate-serialport-4525'

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

Keywords: review added
Owner: Glyph deleted

I mostly just pulled some code out of the #3802 branch. Note in particular how timeout is handled in the affected code.

Also, I don't know how well the new API docs are going to be rendered. The serialport modules are constructed in a weird way, probably confusing to pydoctor.

build results

Changed 11 years ago by Lucas Taylor

Attachment: 4525-1.diff added

Minor modifications

comment:3 Changed 11 years ago by Lucas Taylor

Cc: Lucas Taylor added

Attaching some minor modifications:

  • Added reactor and protocol to the BaseSerialPort initialization and set attributes there since they are common to both implementations. Also, the way BaseSerialPort was being initialized would have resulted in an error due to parameter order.
  • Initially flush serial port in BaseSerialPort
  • Removed an extraneous protocol attribute assignment in _win32serialport.py
  • docstrings for requirements pyserial and pywin32

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

Attaching some minor modifications

Thanks. I'll check this in. Meanwhile, I think #3690 should be reviewed and resolved first, since it introduces the first serial port tests we'll have, and refactoring without unit tests is crazy. There'll be some minor conflicts, easily resolved.

comment:5 Changed 11 years ago by Thijs Triemstra

Those pyserial requirements should probably go in the readme file, not in the module.

comment:6 Changed 11 years ago by Jonathan Lange

Keywords: review removed

Thanks for removing the duplication.

I don't know whether I agree with thijs's comment, that is, I don't know what the correct principle is. From a pragmatic POV, I think the dependency requirements are fine where they are.

This bit:

+    Non-implementation of the serial port transport, only actually used when
+    running on a platform for which Twisted lacks serial port support
+    (currently POSIX and Windows on CPython are supported).

seems a bit clunky, but I can't think of any improvements.

In a nutshell, I'm fine for this to land as-is, pending #3690.

comment:7 Changed 11 years ago by <automation>

Note: See TracTickets for help on using tickets.