[chirp_devel] [ts2000] Feature complete TS-2000 driver. Fixes #217

[Tom Hayward]

On Sun, Mar 29, 2015 at 11:55 AM, Dan Smith <dsmith at danplanet.com> wrote:
>> # HG changeset patch
>> # User Charles Stewart <chuckination at gmail.com>
>> # Date 1427604940 14400
>> #      Sun Mar 29 00:55:40 2015 -0400
>> # Node ID 4e970afe0a97d3d5f52165a1705c848f5e670825
>> # Parent  ee81cedd447b16eacca70ff6e8bff3552e4a5227
>> [ts2000] Feature complete TS-2000 driver.  Fixes #217
>>
>> Debugged Tom Hayward's initial TS-2000 driver and modified
>> kenwood_live.py to support detecting it.
>
> Thanks!
>
> Tom, can you take a look over this before we put it into the tree? Since
> it affects more than just the TS2000 driver, we should try to avoid
> breaking others.

I'm really struggling to read code this morning, but I'll do my best.

The use of "curj" is odd. Some code from the patch:
+    command_delimiters = [("\r"," "), (";","")]
+
     for i in bauds:
+        curj = 0
+        for j in command_delimiters:
+            curj += 1
+            LOG.info("Trying ID at baud %i with delimiter check #%i"
% (i, curj))


In Python, we usually do this with enumerate() if you really need the
index, like this:
    command_delimiters = [("\r"," "), (";","")]
    for i in bauds:
        for curj, j in enumerate(command_delimiters):
            LOG.info("Trying ID at baud %i with delimiter check #%i" %
(i, curj))


However, it looks like you only use the integer index during logging,
so why not make the logs easier to read by logging the actual
delimiter:
    command_delimiters = [("\r"," "), (";","")]
    for i in bauds:
        for j in command_delimiters:
            LOG.info("Trying ID at baud %i with delimiter %s" % (i, j))


Going one more step, you could make the code easier to read with a
descriptive variable name:
    command_delimiters = [("\r"," "), (";","")]
    for i in bauds:
        for delimiter in command_delimiters:
            LOG.info("Trying ID at baud %i with delimiter %s" % (i, delimiter))


You changed a couple double line-breaks between top-level functions to
single line-breaks. According to PEP8, these really should be double
line-breaks. Please revert this.
https://www.python.org/dev/peps/pep-0008/#blank-lines


Why change __memcache to _memcache? This appears to be only a style
change and not affect the behavior of the TS-2000 driver. If this is
true, I'd prefer if you chose one of these alternatives:
1) Leave __memcache the way it is, or
2) Send style changes in a separate patch.


Here's the list of errors from run_all_tests.sh:
chirp/drivers/ts2000.py:19:80: E501 line too long (80 > 79 characters)
chirp/drivers/ts2000.py:87:80: E501 line too long (84 > 79 characters)
chirp/drivers/ts2000.py:156:80: E501 line too long (80 > 79 characters)
chirp/drivers/ts2000.py:201:80: E501 line too long (93 > 79 characters)
chirp/drivers/ts2000.py:235:80: E501 line too long (89 > 79 characters)
chirp/drivers/ts2000.py:236:80: E501 line too long (92 > 79 characters)
chirp/drivers/ts2000.py:237:80: E501 line too long (90 > 79 characters)
chirp/drivers/ts2000.py:278:80: E501 line too long (89 > 79 characters)
chirp/drivers/ts2000.py:279:80: E501 line too long (92 > 79 characters)
chirp/drivers/ts2000.py:280:80: E501 line too long (90 > 79 characters)
FAIL: Please use <80 columns in source files
Checking for PEP8 regressions...
./chirp/drivers/kenwood_live.py:80:1: E302 expected 2 blank lines, found 1
./chirp/drivers/kenwood_live.py:88:32: E231 missing whitespace after ','
./chirp/drivers/kenwood_live.py:88:43: E231 missing whitespace after ','
./chirp/drivers/kenwood_live.py:95:80: E501 line too long (81 > 79 characters)
./chirp/drivers/kenwood_live.py:113:1: E302 expected 2 blank lines, found 1
./chirp/drivers/kenwood_live.py:124:1: E302 expected 2 blank lines, found 1


Thanks for working on this driver! Lots of people have asked for it.

Tom KD7LXL