Opened 12 years ago
Last modified 12 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)
Change History (8)
comment:1 Changed 12 years ago by
Author: | → exarkun |
---|---|
Branch: | → branches/refactor-duplicate-serialport-4525 |
comment:2 Changed 12 years ago by
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.
comment:3 Changed 12 years ago by
Cc: | Lucas Taylor added |
---|
Attaching some minor modifications:
- Added
reactor
andprotocol
to theBaseSerialPort
initialization and set attributes there since they are common to both implementations. Also, the wayBaseSerialPort
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
andpywin32
comment:4 Changed 12 years ago by
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 12 years ago by
Those pyserial requirements should probably go in the readme file, not in the module.
comment:6 Changed 12 years ago by
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.
(In [29429]) Branching to 'refactor-duplicate-serialport-4525'