[chirp_devel] [developer] Control address format for full-file dump/diff - #1767

[chirp.cordless at xoxy.net]

Well, I can see why a Python aficionado would like that. 8-)

My first reaction is concern for error handling, it seems like it
would be pretty easy to specify something with too few or
two many arguments. Yeah, the audience is developers and
they can probably sort out the exception, but it seems like
buying trouble.

The second thing is that my own preferred format is "both"
which sort of breaks your model:
>> out += "%04i x%04X: " % ((i * line_sz), (i * line_sz))

This requires the statement to have two (identical) address arguments
in a list. Handling that would require scanning the supplied format,
and while I'm sure it's computable, it's more work than I'm
interested in, and this flexibility doesn't get me very excited.

If you're OK with letting the user deal with whatever exception
they might cause, I could see handling the case of
"not decimal or hex or both" by assuming one address argument
and just pasting whatever the user supplies for the format.

It doesn't solve any problem I have, but I think it's minimal
work and I could do that. I.e.

Eliminate this:
>> +    elif (addrfmt != "decimal" and addrfmt != "hex" and addrfmt != "both"):
>> +        print "Invalid hexdump_addrfmt value %s. Using decimal." % addrfmt 
>> +        addrfmt = "decimal"

and change the print statements to:
> ...

>> +        elif addrfmt == "both":
>> +            out += "%04i x%04X: " % ((i * line_sz), (i * line_sz))
>> +        elif addrfmt == "decimal":
>> +            out += "%04i: " % (i * line_sz)
	       else
		   out += addrfmt % (i * line_sz)

Thoughts?

-dan

On Jul 15, 2014, at 5:30 PM, Dan Smith - dsmith at danplanet.com wrote:
>> +
>> +    try:
>> +        addrfmt = CONF.get("hexdump_addrfmt", "developer")
>> +    except Exception:
>> +        addrfmt = "decimal"
>> +    if addrfmt == None:
>> +        addrfmt = "decimal"
>> +    elif (addrfmt != "decimal" and addrfmt != "hex" and addrfmt != "both"):
>> +        print "Invalid hexdump_addrfmt value %s. Using decimal." % addrfmt 
>> +        addrfmt = "decimal"
>> 
>>     for i in range(0, (len(data)/line_sz)):
>> -        out += "%03i: " % (i * line_sz)
>> +        if addrfmt == "hex":
>> +            out += "x%04X: " % (i * line_sz)
>> +        elif addrfmt == "both":
>> +            out += "%04i x%04X: " % ((i * line_sz), (i * line_sz))
>> +        else:
>> +            out += "%04i: " % (i * line_sz)
> 
> Just a thought...
> 
> Instead of having them choose from a few different symbolic choices, why
> not just let them specify the format themselves? So in the config:
> 
>   hexdump_addrfmt = %0000x
> 
> and in the code:
> 
>   fmt = CONF.get("hexdump_addrfmt", "developer")
>   . . .
>   thing = fmt % addr
> 
> I can just see someone coming along and wanting octal or something and
> if we just let them specify the format, we can avoid adding new symbols.
> What do you think?
> 
> --Dan