Skip to content

Conversation

@shombando
Copy link

Description

Both general.import and import keys were being created to import the theme despite the version check. This fixes it so only the appropriate key is created based on Alacritty version.

Checklist

  • Change is backwards compatible.

  • Code formatted with nix fmt or
    nix-shell -p treefmt nixfmt deadnix keep-sorted --run treefmt.

  • [-] Code tested through nix run .#tests -- test-all or
    nix-shell --pure tests -A run.all.
    Ran the following: nix-shell --pure tests tests/modules/programs/alacritty

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.
    • Generate a news entry. See News
    • Basic tests added. See Tests
  • If this PR adds an exciting new feature or contains a breaking change.

    • Generate a news entry. See News

Both `general.import` and `import` keys were being created to
import the theme despite the version check. This fixes it so only the
appropriate key is created based on Alacritty version.
general.import = lib.mkIf (lib.versionAtLeast cfg.package.version "0.14") [ theme ];
import = lib.mkIf (lib.versionOlder cfg.package.version "0.14") [ theme ];
};
lib.mkIf (cfg.theme != null) (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update tests to verify the expected behavior of the change. So we can see the expected toml for pre 0.14 and post 0.14.

Also looks like a simpler solution would have been to include the import key in the guarded part no? We keep mkIf benefits and just don't create any of the key/values unless version matches.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @khaneliman I will try to learn how to do the thing you suggested and also how to create a test for this. I have less than rudimentary understanding of Nix so it might take me a long time to find time and figure it out. I am happy to take any pointers/links to examples but have no expectations in having my hand held. Thanks for the suggestions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khaneliman maybe I should be using lib.mkMerge to preserve the benefits of lib.mkIf? If that's not the right path, please let me know. I'll learn about updating the tests and give it a go. I can verify now that it will generate only one attribute based on the version number specified, which is the bug being fixed. Thanks again for suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have various examples in the repo of where the cfg.packge version is checked to determine something. I'd look for something that uses mkStubPackage with a version = "" that mocks up a specific version of a package to test version logic in a module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants