On 2021-05-24 10:41:18+0100, Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > Hi Lénaïc > > On 24/05/2021 08:15, Lénaïc Huard wrote: > > The first parameter of `XXX_update_schedule` and alike functions is a > > boolean specifying if the tasks should be scheduled or unscheduled. > > > > Using an `enum` with `ENABLE` and `DISABLE` values can make the code > > clearer. > > I'm sorry to say that I'm not sure this does make the code clearer overall - > I wish I'd spoken up when Danh suggested it. > While > launchctl_boot_plist(DISABLE, filename, cmd) > is arguably clearer than > launchctl_boot_plist(0, filename, cmd) > we end up with bizarre tests like > if (enabled == ENABLED) > rather than > if (enabled) > and in the next patch we have > (enable == ENABLE && (opts->scheduler == i)) ? > ENABLE : DISABLE; > rather than > enable && opts->scheduler == i > > Also looking at the next patch it seems as this one is missing some > conversions in maintenance_start() as it is still calling > update_background_schedule() with an integer rather than the new enum. Yes, in this form, I also think the change looks bizarre. And, it's entirely my fault. I also agree with Ævar that 0 and 1 is meant well for off/on. However, I still think launchctl_boot_plist(0, filename, cmd) would require some degree on code navigation to figure out what would that LoC does. I'm thinking about rename the function. But, it would trigger a forever bikeshedding, which shouldn't be a blocker for this series. > I'd be happy to see this being dropped I'm afraid So, let's drop this patch and start a new conversation when the dust settled. -- Danh
