[chirp_devel] [FT-90] add initial support for FT-90R. #1087

[Jens J.]

Ok, thanks for pointers on type consistency in the steps list. (I missed your point from an earlier email.)
No good reason other than than being new to python development ;)

I would point out in the spirit of constructive criticism, that a. there is some inconsistency in this and other regards amongst various modules, and b. there is a lack clear developer documentation on style (such as "please follow PEP8 when reasonable") guidelines, and project-level conventions, etc.
I do understand the need for consistency, and that this is a community project, so that's why I would suggest some and even be willing to help with this on the twiki (although my available time is about to plummet due to school semester starting).
With all of that aside, changes made - please review latest version of the patch.
:)
Jens



________________________________
 From: Dan Smith <dsmith at danplanet.com>
To: chirp_devel at intrepid.danplanet.com 
Sent: Monday, August 26, 2013 9:34 AM
Subject: Re: [chirp_devel] [FT-90] add initial support for FT-90R. #1087
 

> +    STEPS = [5, 10, 12.5, 15, 20, 25, 50]

While I understand this works, it's not what the other drivers do.
Please make these floats:

  STEPS = [5.0, 10.0, ...]

Just for consistency, if you don't mind.

> +    MODES = ["AM", "FM", "Auto"]
> +    TMODES = ["", "Tone", "TSQL", "", "DTCS"] # idx 3 (Bell) not ...
> +    TONES = list(chirp_common.TONES)
> +    for tone in [ 165.5, 171.3, 177.3 ]:
> +        TONES.remove(tone)        
> +    POWER_LEVELS = ["Hi", "Mid1", "Mid2", "Low"]
> +    DUPLEX = ["", "-", "+", "split"]
> +    mem_format = """

Sorry, I didn't notice this before, but I would really prefer that these
be module-level and not public class-level variables. Is there some
reason you moved these into the class, contrary to the pattern used
everywhere else? Class variables with mutable types are pretty
dangerous in Python...

I'm not trying to be overly pedantic here, just trying to keep
consistency with everything else. You're *really* close; thanks for
your work and patience on this :)

-- 
Dan Smith
www.danplanet.com
KK7DS
_______________________________________________
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/20130826/d2c0dc20/attachment-0001.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ft90init.patch
Type: text/x-patch
Size: 12189 bytes
Desc: not available
Url : http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20130826/d2c0dc20/attachment-0001.bin