On Thursday, 25 August 2016 17:36:14 CEST Richard W.M. Jones wrote:
> On Thu, Aug 25, 2016 at 06:05:16PM +0200, Pino Toscano wrote:
> > +(* Grub2 representation. *)
> > +class bootloader_grub2 (g : G.guestfs) grub_config =
> > +  let grub2_update_console ~remove =
> 
> I checked the before and after code and I don't think anything has
> been missed out.
> 
> My only comment is it seems a bit awkward stuffing the
> grub2_update_console function definition into what is effectively the
> class constructor.  Maybe it should be hoisted to the top level of the
> Linux_bootloaders module (not exported, of course), or turned into a
> private method of the class?

Indeed -- I turned that into a proper private class method, so it is
not available outside the bootloader_grub2 class.

> Anyway as this is just a quibble, ACK.

Quibbling is fine :)

Pushed with the aforementioned change.

Thanks for the thorough review,
-- 
Pino Toscano

Attachment: signature.asc
Description: This is a digitally signed message part.

_______________________________________________
Libguestfs mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libguestfs

Reply via email to