Re: [chirp_devel] [PATCH] Add import support for Kenwood *.hmk files

diff -r 91be43cc7ac4 -r 1bb3df3d624f chirp/directory.py --- a/chirp/directory.py Tue Apr 03 11:19:10 2012 -0700 +++ b/chirp/directory.py Tue Apr 03 16:21:35 2012 -0600 @@ -91,6 +91,9 @@ if image_file.lower().endswith(".csv"): return get_radio("Generic_CSV")(image_file)

• if image_file.lower().endswith(".hmk"):

•    return get_radio("Kenwood_HMK")(image_file)
  

• if icf.is_9x_icf(image_file): return get_radio("Icom_IC91_92AD_ICF")(image_file)

This follows the existing pattern, but I think the existing pattern is ugly. I can say that, since it's mine.

I suggested on IRC that you use match_model(), and then realized when I saw the above that match_model() isn't used for CSV (or chirp) files, and can't be for extension-based matching. What was I thinking.

I'll send a patch to the list in a minute that will enable doing so, and convert the three cases there to use match_model() instead. Let me know what you think about doing it that way instead.

+class HMKRadio(chirp_common.CloneModeRadio):

I was thinking you would inherit from CSVRadio and hopefully eliminate the need to duplicate all that work. Was there some reason this didn't work out?

• def _blank(self):

•    self.errors = []
  

•    self.memories = []
  

•    for i in range(0, 1000):
  

•        m = chirp_common.Memory()
  

•        m.number = i
  

•        m.empty = True
  

•        self.memories.append(m)
  

I don't think you need _blank() if you're not going to support editing, right? If you inherit it from CSVRadio, it's one thing, but I think it'll confuse a future developer if it's here for no reason.

• def __init__(self, pipe):

•    chirp_common.CloneModeRadio.__init__(self, None)
  

•    self._filename = pipe
  

•    if self._filename and os.path.exists(self._filename):
  

•        self.load()
  

•    else:
  

•        self._blank()
  

It's an error if we're instantiated without a filename.

• def get_features(self):

•    rf = chirp_common.RadioFeatures()
  

•    rf.has_bank = False
  

•    rf.has_dtcs_polarity = False
  

•    rf.memory_bounds = (0, len(self.memories))
  

•    rf.has_infinite_number = True
  

•    rf.valid_modes = list(chirp_common.MODES)
  

•    rf.valid_tmodes = list(chirp_common.TONE_MODES)
  

•    rf.valid_duplexes = ["", "-", "+", "split"]
  

•    rf.valid_tuning_steps = list(chirp_common.TUNING_STEPS)
  

•    rf.valid_bands = [(1, 10000000000)]
  

•    rf.valid_skips = ["", "S"]
  

•    rf.valid_characters = chirp_common.CHARSET_ASCII
  

•    rf.valid_name_length = 999
  

•    return rf
  

If you inherit from CSVRadio, then these are fine, but I think they are probably unnecessary if you're import-only, right?

• def _parse_quoted_line(self, line):

•    line = line.replace("\n", "")
  

•    line = line.replace("\r", "")
  

•    line = line.replace('"', "")
  

•    return line.split(",")
  

This is dead code from the CSV driver. Lets remove it from there and not replicate it here :)

• def _parse_csv_data_line(self, headers, line):

•    mem = chirp_common.Memory()
  

•    odd_split = False
  

•    for header, (typ, attr) in self.ATTR_MAP.items():
  

•        try:
  

•            val = self._get_datum_by_header(headers, line, header)
  

•            if not val and typ == int:
  

•                val = None
  

•            elif attr == "duplex":
  

•                val = typ(self.DUPLEX_MAP[val])
  

•                if val == "split":
  

•                    odd_split = True
  

Hmm, what do the Tx Freq. and Offset columns look like in the two cases? Seems like we should be able to do better than this.

•            elif attr == "skip":
  

•                val = typ(self.SKIP_MAP[val])
  

•            elif attr == "tmode":
  

•                val = typ(self.TMODE_MAP[val])
  

Instead of special-casing these, why not use a function like the frequency parsing case? Something like this:

ATTR_MAP = { ... "T/CT/DCS" : (lambda v: self.TMODE_MAP[v], "tmode"), "L.Out" : (lambda v: self.SKIP_MAP[v], "skip"), ... }

That way you can just cover all the cases with this:

•            else:
  

•                val = typ(val)
  

...like the CSV driver does. Plus, if we can figure out something sneaky for the Offset and Tx Freq. cases, then you could still inherit from CSVRadio with a different ATTR_MAP :)

• def load(self, filename=None):

•    if filename is None and self._filename is None:
  

•        raise errors.RadioError("Need a location to load from")
  

•    if filename:
  

•        self._filename = filename
  

•    self._blank()
  

I think you can remove this.

•    f = file(self._filename, "r")
  

•    for i in range(0, 10):
  

•        f.readline().strip()
  

Is it always ten lines? Maybe it would be better to probe smartly to chew up everything until the header row?

•    #f.seek(0, 0)
  

Did you intend to leave this in or take it out?