Re: [libvirt] [PATCH 1/2] Introduce virDomainYesNo enum type

On 07/14/2014 10:40 AM, Daniel P. Berrange wrote: >>>> } >>>> - def->os.bios.useserial = VIR_DOMAIN_BIOS_USESERIAL_YES; >>>> + def->os.bios.useserial = VIR_DOMAIN_YES_NO_ENABLED; >>>> } else { >>>> - def->os.bios.useserial = VIR_DOMAIN_BIOS_USESERIAL_NO; >>>> + def->os.bios.useserial = VIR_DOMAIN_YES_NO_DISABLED; >>>> } >>>> if (def->data.spice.filetransfer) >>>> virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n", >>>> - virDomainGraphicsSpiceAgentFileTransferTypeToString(def->data.spice.filetransfer)); >>>> + virDomainYesNoTypeToString(def->data.spice.filetransfer)); >>>> } >>> >>> I'm not really a fan of this cleanup, as IMHO the result is less clear & >>> harder to follow than the original code. >> >> How so? The original code was very repetitive, with multiple enums (all >> with long names) copying the same few enum elements. We're not painting >> ourselves into a corner - if any of the replaced enums ever grows a >> third value (such as "on", "hybrid", "off"), then we just break that one >> enum back into a named list rather than using the generic on/off enum. >> I'm actually in favor of this cleanup. > > Specifically a enum constant name like YES_NO_DISABLED is just awful IMHO > compared to the original desriptive name.

Is it just a matter of coming up with a better name? Maybe: VIR_TRISTATE_ABSENT = 0, VIR_TRISTATE_NO, VIR_TRISTATE_YES,

def->os.bios.useserial = VIR_TRISTATE_NO; if (def->data.spice.filetransfer) { virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n", virTristateToString(def->data.spice.filetransfer));