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

On 2021-05-24 10:41:18+0100, Phillip Wood [off-list ref] wrote:

Hi Lénaïc

On 24/05/2021 08:15, Lénaïc Huard wrote:

quoted

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