Miscellaneous: Don't use mkEnableOption

[chris-martin]

This removes usages of mkEnableOption (#18767).

This is a work in progress. There are about a hundred more usages.

[mention-bot]

@chris-martin, thanks for your PR! By analyzing the annotation information on this pull request, we identified @Profpatsch, @matthewbauer and @ehmry to be potential reviewers

[copumpkin]

I'm ambivalent on this, but the "Fairbairn threshold" is a relevant term here: https://mail.haskell.org/pipermail/libraries/2012-February/017548.html

[edwtjo]

Are we not squashing commits anymore? Since this is such a sweeping change, make sure that it is done before merge/cherry-pick.

[chris-martin]

Agh, I wish I'd known that before I went through the effort to put these in separate commits. I thought the guideline was to separate commits affecting different packages.

[edwtjo]

@chris-martin well, I'm not an absolutist, if it makes sense to "downgrade" expressions individually then separate commits are IMHO always in order. In this case however I don't think there is any reason for that, either we use mkEnableOption or not. Also for this PR to be complete you should take a pass over the documentation and make appropriate changes.

[chris-martin]

By the way, I've already found a handful of non-obvious mistakes such as

enable = mkEnableOption "Whether to run the rspamd daemon.";

[rasendubi]

Should we remove mkEnableOption from lib. or make it deprecated?

[groxxda]

I'd say deprecate it at first

example = !default should be automatically inferred for every bool. Is this hard to do? 😉

[Profpatsch]

Wow, I really don’t like this change. I see no positive effect whatsoever, except for adding four lines of code to stuff that was pretty self-descriptive before. Now I’ll have to look up what argument fields options want instead, and will forget to set the default value half of the time.

Strong 👎.

[domenkozar]

@Profpatsch for positive effects, please read #18767

Note that you need to know argument fields options if you want to write NixOS modules. This change doesn't change this fact. And you still need understand mkIf cfg.enable, regardless.

We should make NixOS easier. Whole Nix language is designed on the fact it's a simple DSL for anyone to understand.

[RonnyPfannschmidt]

note that easy and simple are not the same thing - this chnage makes things more simple

the cost is removing a simple call the benefit is having a consistent comprehensible option pattern

[groxxda]

@Profpatsch there are different ways to look at this:
I'll list pros and cons of mkEnableOption (keep in mind that code is read more often than it's written (hopefully)):

Reading code

➕ read 1 instead of 4 lines
➖ have to know the definition of mkEnableOption (if you don't know it, find the definition and look it up only to see its "pretty self-descriptiveness")

Writing code

➕ write 1 instead of 4 lines if all default values are acceptable
➕ cannot forget (optional) attributes default, type and example (All other options you create should have them as well, so why would you forget them only for the enable option!?)
➖ defaults may not be suitable:

• use mkEnableOption but override (e.g. mkEnableOption "abc" // { description = "blabla"; }
• use mkOption instead

➖ have to know about mkEnableOption and remember to use it

• otherwise there's extra work for the reviewer and the original author
• or we introduce more inconsistency into our codebase

~/nixpkgs ag mkEnableOp nixos/modules | wc -l                                                                   
182
~/nixpkgs ag enable -A4 nixos/modules | ag  'type.*bool' | wc -l                                                        
397

[Profpatsch]

I see the reasoning. Maybe it’s my gripe with the verbosity of module declaration in general, I still think there’s an abstraction slumbering in there that we should create. Editor templates can never be the solution imo.

[chris-martin]

Another idea: What if we add a value like

enableOptArgs = {
  default = false;
  example = true;
  type = types.bool;
};

Then you can write

enable = mkOption (enableOptArgs // { description = "Whether to enable to xyz service."; });

That way you get both:

• All options in the repo will be defined using mkOption directly
• Most cases will still be one line

[groxxda]

@chris-martin that would trade the extra function for an extra constant that one had to know / lookup. Imho that would be even worse..

[copumpkin]

@groxxda how is it different from having to look up most of the other functions people have put in lib, most of which have short and fairly succinct definitions that someone factored out because they were used in many places?

As I said, I'm mostly ambivalent about this, but it seems like a weird line to draw in the sand and declare awful. To me it's on the edge of a useful abstraction. Back when we had goPackages, I noticed that 99% of the go packages were the same call to buildGoPackage and fetchFromGitHub and pulling out the first 8 characters of the sha1 revision, so I gave that thing a name and saved everyone a ton of work writing the same composition of basic error-prone functionality every time. Yes, there was a tiny amount of lookup overhead, but ultimately people just pattern match when they write this stuff and I don't think mkEnableOption changes that.

Ultimately, I don't understand why people seem to care so much about something that feels so bikesheddy. If consensus is to remove it, I won't fight it, but it doesn't honestly seem that bad to me.

[chris-martin]

If we had an API reference page, I'd be happier with #18526.

[jagajaga]

I am against this change.

We can write programs in any language with only basic functions and primitives. But we can also use libs and make our code easier to use and read. The same thing is with mkEnableOption. If it's hard for you to use it -- don't use it, but please don't think that everyone is like you. Learn nix and start write better nix code.
If we are talking about newbies then we must deprecate any use of any lib in any language, because "it will be hard for them to read and understand the code!". That's pretty dumb reason.

[chris-martin]

If we are talking about newbies then we must deprecate any use of any lib in any language

We're already talking about which side of the Fairbairn threshold this lies on - I don't think the absurdity is helpful.

[jagajaga]

@chris-martin this function makes code better and easier to read. Also it motivates to read nix code a bit.

This proposal is bad and I would like to reopen #18526 and merge it.

[ericsagnes]

@jagajaga

this function makes code better and easier to read.

This is a very subjective statement, having enable options the only ones declared in a different manner can be "worse" and "harder" to read for some, as modules declaration lose uniformity.

If it's hard for you to use it -- don't use it

This proposal is bad and I would like to reopen #18526 and merge it.

This 2 affirmations sounds a bit contradictory, it is ok to not use mkEnableOption but you want to make mkEnableOption used in all modules?

[langston-barrett]

Let's continue the discussion on #18767, then decide which of these PRs to reopen and merge.