Fix "i18n.inputMethod.enabled has a confusing type" #310708
Conversation
|
This is a breaking change for 24.05 (hence the documentation update). |
|
I think you meant |
| @@ -28,8 +30,10 @@ in | |||
| { | |||
| options.i18n = { | |||
| inputMethod = { | |||
| enabled = mkOption { | |||
| type = types.nullOr (types.enum [ "ibus" "fcitx5" "nabi" "uim" "hime" "kime" ]); | |||
| enabled = mkEnableOption "an additional input method type"; | |||
There was a problem hiding this comment.
| enabled = mkEnableOption "an additional input method type"; | |
| enable = mkEnableOption "an additional input method type"; |
for consistency with any other module.
Then we also add a mkRenamedOptionModule entry to imports
Also I'd adjust the defaults for the new options, for a smoother transition, i.e. set enable's default to enabled != null and type's default to enabled
There was a problem hiding this comment.
I've made both of these changes, but they seem to be mutually exclusive, and the two tests are now failing.
Am I just doing it wrong? I'd appreciate your guidance.
Test with new style config (nix-build -A nixosTests.installed-tests.ibus), where the error is in the module definition itself:
error:
Failed assertions:
- The option definition `i18n.inputMethod.enabled' in `/home/gy/src/nixpkgs/nixos/modules/i18n/input-method/default.nix' no longer has any effect; please remove it.
Use <literal>i18n.inputMethod.type and .enable = true; instead<literal>
Test with old style config (nix-build -A nixosTests.fcitx5):
error: The option `i18n.inputMethod.enabled' can no longer be used since it's been removed. Use <literal>i18n.inputMethod.type and .enable = true; instead<literal>
There was a problem hiding this comment.
You're right. We should not use mk(Removed|Renamed)OptionModule here.
Removing that fixes the tests.
What we could do (to ensure that users get a warning with .enabled) is to set config.warnings if .enabled != null.
|
Drafting for now as breaking changes are frozen anyway due to the release (see #303285) |
I don't feel the need to rush this in, but if it is ready before the 15th: with your suggestions the change is no longer breaking so could be included in 24.05? |
|
Yes, if we're not breaking anything, I believe it could even be backported per https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#changes-acceptable-for-releases:
|
| @@ -11,7 +11,8 @@ On NixOS, you need to explicitly enable `ibus` with given engines before customi | |||
| ```nix | |||
| { pkgs, ... }: { | |||
| i18n.inputMethod = { | |||
| enabled = "ibus"; | |||
There was a problem hiding this comment.
I've intentionally left the other test (nixos/tests/fcitx5/default.nix, not present in this review) using enabled to have a test for the backwards compatibility of this change
There was a problem hiding this comment.
Whoops, this is the docs file. I meant to add this to nixos/tests/installed-tests/ibus.nix
| @@ -81,7 +82,8 @@ The following snippet can be used to configure Fcitx: | |||
| ```nix | |||
| { | |||
| i18n.inputMethod = { | |||
| enabled = "fcitx5"; | |||
There was a problem hiding this comment.
Although I've updated the documentation for all of the input methods
This commit introduces two new properties: `enable` and `type`, to replace the `enabled` property. `enable` has the same meaning as is common across nixpkgs. `type` has the same meaning as the existing `enabled` property. `enabled` property is now deprecated and will be removed in a future release. Fixes NixOS#180654
|
@eclairevoyant this is ready to go |
Description of changes
This commit introduces two new properties:
enableandtype, to replace theenabledproperty.enablehas the same meaning as is common across nixpkgs.typehas the same meaning as the existingenabledproperty.enabledproperty is now deprecated and will be removed in a future release.Fixes #180654
I updated and ran the existing inputMethod tests:
nix-build -A nixosTests.fcitx5 -A nixosTests.installed-tests.ibusMaintainers:
@ericsagnes
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.