Code review comment for lp:~dooferlad/linaro-image-tools/add-directory-option

On Tue, Apr 17, 2012 at 3:37 PM, Loïc Minier <email address hidden> wrote:
>> +def prep_media_path(args, board_config):
>> +
>> +    # If args.device isn't set, generate a sensible default file name and
>> +    # tell the user
>> +    if not args.device:
>> +        args.device = board_config.board + ".img"
>> +        print "Setting destination file name to", args.device
>
>  I think it would be more stable to just call it mmc.img or sd.img.

+1, that would help scripting the output later on.

>> +    if args.directory is not None:
>> +        # If args.device is a path to a device (/dev/) then this is an error
>> +        if "--mmc" in sys.argv:
>> +            raise IncompatableOptions("--directory option incompatable with "
>> +                                      "option --mmc")
>> +
>> +        # If directory is used as well as having a full path (rather than just
>> +        # a file name or relative path) in args.device, this is an error.
>> +        if re.search(r"^/", args.device):
>> +            raise IncompatableOptions("--directory option incompatable with "
>
>  IncompatibleOptions + "incompatible"
>
>  We could actually support absolute --image-file with --directory and
>  just output the image where the user wants it, but I don't really care
>  strongly about the behavior in that particular use case.
>
>  Generally ok with the design apart of the minor comments here, but
>  I have one objection: I think the argument validation / mangling should
>  happen when parsing the cmdline rather than in boards code.

+1

>  Otherwise good, thanks!

+1