Libguestfs - Jan 2014 - Re: [Bug 1046905] New: RFE: add argument to virt-sysprep to disable individual default operations

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.
> +        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
  ...
> +          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.
> +    "--operations", Arg.String set_operations, " " ^
s_"Enable/disable specific operations";
I'd also add an alias "--operation" (it would just do the same
thing).
> +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

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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.
> 
> > +        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
>   ...
An even more natural way to write this is:

  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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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