[chirp_devel] [PATCH 5/6] Improve CLI download/upload (#2343)

[Zach Welch]

On 03/05/2015 03:22 PM, Dan Smith wrote:
...
>> +            LOG.error("You must specify the destnation file name with --mmap")
>
> Typo in 'destination'.

Good catch.

>> +            sys.exit(1)
>> +        try:
>> +            radio.sync_in()
>> +            radio.save_mmap(options.mmap)
>> +        except Exception, e:
>> +            LOG.error(e)
>
> This is going to make it really hard to debug what's going on if/when
> someone reports it. LOG.exception() will dump the exception, but at
> ERROR level, which is probably not what you want here. Maybe check your
> global --debug flag and only do it in that case?

For now, I think we need to use LOG.exception unconditionally.  We can't 
use the new is_visible for log messages.  That routine only checks the 
console's verbosity level.  As such, it is only intended to control 
normal program output, since that does not go through the logging 
infrastructure.

For logging, we need to rely on the handlers to set their desired 
logging level (via --verbose/--quiet).  Our code unconditionally makes 
the proper call, and the handlers sort out whether to display it. 
Concretely, the console and log file may have different logging levels 
(by default, WARN and DEBUG respectively), so a message may show up in 
one and not the other.  In other words, log output is already 
conditional on the logging level; it would be redundant (at best) and 
confusing (at worst) to try to add another condition to it.

Down the road, I think we should plan to switch it to LOG.error, because 
other logging information (generated at the point of exception) should 
provide sufficient context for us to debug things.  More generally, 
users should never see a stack trace (in an ideal world).  Of course, we 
are nowhere near that goal as yet, so LOG.exception is the right thing 
to do.

>> +        sys.exit(1)
>>
>>       if options.upload_mmap:
>> -        # isinstance(radio, chirp_common.IcomMmapRadio) or fail_unsupported()
>> -        radio.load_mmap(options.mmap)
>> -        if radio.sync_out():
>> -            print "Clone successful"
>> -        else:
>> -            LOG.error("Clone failed")
>> +        if not issubclass(rclass, chirp_common.CloneModeRadio):
>> +            LOG.error("%s is not a clone mode radio" % options.radio)
>> +            sys.exit(1)
>> +        if not options.mmap:
>> +            LOG.error("You must specify the destnation file name with --mmap")
>
> Same typo here.

Actually, it's even worse; it should read "source", not "destination".

Cheers,
-- 
Zach Welch
Mandolin Creek Farm
www.mandolincreekfarm.com
farm: 541-453-4131
cell: 541-740-3410