<div dir="ltr"><div>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.</div><div><br></div><div>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.</div></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature"><div dir="ltr">Charles Stewart<br><a href="mailto:ccstewart@gmail.com" target="_blank">chuckination@gmail.com</a></div></div></div>
<br><div class="gmail_quote">On Mon, Mar 30, 2015 at 2:13 PM, Tom Hayward <span dir="ltr"><<a href="mailto:tom@tomh.us" target="_blank">tom@tomh.us</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Sun, Mar 29, 2015 at 11:55 AM, Dan Smith <<a href="mailto:dsmith@danplanet.com">dsmith@danplanet.com</a>> wrote:<br>
>> # HG changeset patch<br>
>> # User Charles Stewart <<a href="mailto:chuckination@gmail.com">chuckination@gmail.com</a>><br>
>> # Date 1427604940 14400<br>
>> # Sun Mar 29 00:55:40 2015 -0400<br>
>> # Node ID 4e970afe0a97d3d5f52165a1705c848f5e670825<br>
>> # Parent ee81cedd447b16eacca70ff6e8bff3552e4a5227<br>
>> [ts2000] Feature complete TS-2000 driver. Fixes #217<br>
>><br>
>> Debugged Tom Hayward's initial TS-2000 driver and modified<br>
>> kenwood_live.py to support detecting it.<br>
><br>
> Thanks!<br>
><br>
> Tom, can you take a look over this before we put it into the tree? Since<br>
> it affects more than just the TS2000 driver, we should try to avoid<br>
> breaking others.<br>
<br>
</div></div>I'm really struggling to read code this morning, but I'll do my best.<br>
<br>
The use of "curj" is odd. Some code from the patch:<br>
+ command_delimiters = [("\r"," "), (";","")]<br>
+<br>
for i in bauds:<br>
+ curj = 0<br>
+ for j in command_delimiters:<br>
+ curj += 1<br>
+ LOG.info("Trying ID at baud %i with delimiter check #%i"<br>
% (i, curj))<br>
<br>
<br>
In Python, we usually do this with enumerate() if you really need the<br>
index, like this:<br>
command_delimiters = [("\r"," "), (";","")]<br>
for i in bauds:<br>
for curj, j in enumerate(command_delimiters):<br>
LOG.info("Trying ID at baud %i with delimiter check #%i" %<br>
(i, curj))<br>
<br>
<br>
However, it looks like you only use the integer index during logging,<br>
so why not make the logs easier to read by logging the actual<br>
delimiter:<br>
command_delimiters = [("\r"," "), (";","")]<br>
for i in bauds:<br>
for j in command_delimiters:<br>
LOG.info("Trying ID at baud %i with delimiter %s" % (i, j))<br>
<br>
<br>
Going one more step, you could make the code easier to read with a<br>
descriptive variable name:<br>
command_delimiters = [("\r"," "), (";","")]<br>
for i in bauds:<br>
for delimiter in command_delimiters:<br>
LOG.info("Trying ID at baud %i with delimiter %s" % (i, delimiter))<br>
<br>
<br>
You changed a couple double line-breaks between top-level functions to<br>
single line-breaks. According to PEP8, these really should be double<br>
line-breaks. Please revert this.<br>
<a href="https://www.python.org/dev/peps/pep-0008/#blank-lines" target="_blank">https://www.python.org/dev/peps/pep-0008/#blank-lines</a><br>
<br>
<br>
Why change __memcache to _memcache? This appears to be only a style<br>
change and not affect the behavior of the TS-2000 driver. If this is<br>
true, I'd prefer if you chose one of these alternatives:<br>
1) Leave __memcache the way it is, or<br>
2) Send style changes in a separate patch.<br>
<br>
<br>
Here's the list of errors from run_all_tests.sh:<br>
chirp/drivers/ts2000.py:19:80: E501 line too long (80 > 79 characters)<br>
chirp/drivers/ts2000.py:87:80: E501 line too long (84 > 79 characters)<br>
chirp/drivers/ts2000.py:156:80: E501 line too long (80 > 79 characters)<br>
chirp/drivers/ts2000.py:201:80: E501 line too long (93 > 79 characters)<br>
chirp/drivers/ts2000.py:235:80: E501 line too long (89 > 79 characters)<br>
chirp/drivers/ts2000.py:236:80: E501 line too long (92 > 79 characters)<br>
chirp/drivers/ts2000.py:237:80: E501 line too long (90 > 79 characters)<br>
chirp/drivers/ts2000.py:278:80: E501 line too long (89 > 79 characters)<br>
chirp/drivers/ts2000.py:279:80: E501 line too long (92 > 79 characters)<br>
chirp/drivers/ts2000.py:280:80: E501 line too long (90 > 79 characters)<br>
FAIL: Please use <80 columns in source files<br>
Checking for PEP8 regressions...<br>
./chirp/drivers/kenwood_live.py:80:1: E302 expected 2 blank lines, found 1<br>
./chirp/drivers/kenwood_live.py:88:32: E231 missing whitespace after ','<br>
./chirp/drivers/kenwood_live.py:88:43: E231 missing whitespace after ','<br>
./chirp/drivers/kenwood_live.py:95:80: E501 line too long (81 > 79 characters)<br>
./chirp/drivers/kenwood_live.py:113:1: E302 expected 2 blank lines, found 1<br>
./chirp/drivers/kenwood_live.py:124:1: E302 expected 2 blank lines, found 1<br>
<br>
<br>
Thanks for working on this driver! Lots of people have asked for it.<br>
<br>
Tom KD7LXL<br>
<div class="HOEnZb"><div class="h5">_______________________________________________<br>
chirp_devel mailing list<br>
<a href="mailto:chirp_devel@intrepid.danplanet.com">chirp_devel@intrepid.danplanet.com</a><br>
<a href="http://intrepid.danplanet.com/mailman/listinfo/chirp_devel" target="_blank">http://intrepid.danplanet.com/mailman/listinfo/chirp_devel</a><br>
Developer docs: <a href="http://chirp.danplanet.com/projects/chirp/wiki/Developers" target="_blank">http://chirp.danplanet.com/projects/chirp/wiki/Developers</a><br>
</div></div></blockquote></div><br></div>