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

[Chucks]

The __memcache to _memcache change was necessary for the TS2000Radio class
to be able to access the KenwoodLiveRadio superclass's member variable for
the subclass's get_memory(), set_memory(), and erase_memory() methods and
cannot be reverted without impacting functioning of the driver.  The double
underscore is analogous to java/C++ private and the single underscore is
analogous to protected, and it's part of the python black magic that
appears stylistic but isn't.  I really hate that particular bit of the
language because it does nothing but cause problems, but there's no such
thing as a language without warts.

Roger on the PEP-8 points.  I printed the index of the delimiters instead
of themselves since the KenwoodLive uses line breaks, but looks like I
could have used repr(delim) instead and it wouldn't have trashed the logs
with the carriage return.  That's why I initially printed the index instead
of the delimiter itself.  Thanks for the tip!  I'll send style changes in
another patch when I have an opportunity to make the clean ups.

Charles Stewart
chuckination at gmail.com <ccstewart at gmail.com>

On Mon, Mar 30, 2015 at 2:13 PM, Tom Hayward <tom at tomh.us> wrote:

> 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
> _______________________________________________
> chirp_devel mailing list
> chirp_devel at intrepid.danplanet.com
> http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
> Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20150401/73eb14ff/attachment-0001.html