Re: [Libguestfs] [Bug 1046905] New: RFE: add argument to virt-sysprep to disable individual default operations
On Friday 10 January 2014 10:09:19 Richard W.M. Jones wrote:
On Thu, Jan 09, 2014 at 03:45:54PM +0000, Richard W.M. Jones wrote:
> On Thu, Jan 09, 2014 at 04:21:10PM +0100, Pino Toscano wrote:
> > + and set_operations op_string =
> > + let currentopset =
> > + match (!operations) with
>
> No need for parentheses around !operations.
Fixed.
> > + let n = ref op_name in
> > + let remove = string_prefix op_name "-" in
> > + if remove then
> > + n := String.sub op_name 1 (String.length op_name - 1);
> > + match !n with
>
> This can be written a bit more naturally as ...
>
> let n, remove =
> if string_prefix op_name "-" then
> String.sub op_name 1 (String.length op_name-1), true
> else
> op_name, false in
> match n with
> ...
This is nice even without the way below.
An even more natural way to write this is:
Maybe for skilled OCaml programmers ;) not (yet) for me.
let op =
if string_prefix op_name "-" then
`Remove (String.sub op_name 1 (String.length op_name-1))
else
`Add op_name
match op with
| `Add "" | `Remove "" -> (* error *)
| `Add "defaults" -> Sysprep_operation.add_defaults_to_set opset
| `Remove "defaults" -> Sysprep_operation.remove_defaults_from_set
| opset etc
The type of op will be [ `Add of string | `Remove of string ].
Actually you never need to write that type out, as OCaml will infer it
(and check it). It will only appear in error messages if you get the
code wrong.
Interesting way, made use of it, thanks.
> + let f = if remove then
> Sysprep_operation.remove_defaults_from_set else
> Sysprep_operation.add_defaults_to_set in
> + f opset
You can actually write:
(if remove then Sysprep_operation.remove_defaults_from_set
else Sysprep_operation.add_defaults_to_set) opset
I don't know which way you think is clearer, but I would avoid the >80
character lines.
Yes, I knew about that, just thought it was clearer to split the actual
call on an own line, making the argument a bit less "hidden" by the
whole instruction.
> + "--operations", Arg.String set_operations, "
" ^
> s_"Enable/disable specific operations";
I'd also add an alias "--operation" (it would just do the same thing).
Added.
> +let add_list_to_opset l s =
> + List.fold_left (
> + fun acc elem ->
> + OperationSet.add elem acc
> + ) s l
> +
> +let remove_list_from_opset l set =
> + OperationSet.fold (
> + fun elem opset ->
> + if List.exists (
> + fun li ->
> + li.name = elem.name
> + ) l then
> + opset
> + else
> + OperationSet.add elem opset
> + ) set empty_set
OCaml Set has subset, intersection, difference operations if you need
them. See /usr/lib64/ocaml/set.mli
True; I went with manual folding to avoid build new temporary sets when
needed, but I guess doing this could be more natural.
--
Pino Toscano