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

From: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Date: May 24, 2021, 12:36 UTC
Message-ID: <YKudvp0m3m+Yi1+m@danh.dev>
In-Reply-To: <8e013441-08dc-fbb5-f9b9-649b2ffd78db@gmail.com>

On 2021-05-24 10:41:18+0100, Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 28 quoted lines

> 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