Subject: Re: [PATCH v4 2/4] maintenance: introduce ENABLE/DISABLE for code clarity

From: Đoàn Trần Công Danh <[email-masked]>
Date: Mon, 24 May 2021 19:36:14 +0700

On 2021-05-24 10:41:18+0100, Phillip Wood <[email-masked]> 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