Skip to content

Conversation

@stasjok
Copy link
Contributor

@stasjok stasjok commented Jul 21, 2024

helpers.mkIfNonNull' checks only for value = null, but I noticed that when there is anywhere something like mkIf false config.filename = {} (like with vlang filetype currently in nixvim, see also #1907), value of config.filetype is not null, but has all attrs with null values. It can be debugged with nix repl:

nix-repl> c = makeNixvim {}

nix-repl> :p c.options.filetype.definitions
[ { extension = { _type = "if"; condition = false; content = { v = "vlang"; }; }; } ]

nix-repl> :p c.config.filetype
{ extension = null; filename = null; pattern = null; }

Notice that condition = false, but the value is not null.

This PR changes mkIf condition to account for that.

NOTE: Test checks.x86_64-linux.tests.entries.modules-filetypes will fail until #1907 is merged.

@MattSturgeon
Copy link
Member

noticed that when there is anywhere something like mkIf false config.filename = {}, value of config.filetype is not null, but has all attrs with null values.

This is probably related to how the module system handles things; e.g. if filetype contains sub-options those options will always appear in the final merged config.

For our use-case where we often want to print entire attrs as lua this is annoying and is why toLuaObject filters out empty attrs by default.

When setting any filetype suboption to null (or anything else guarded by
mkIf) it's value becomes:

    { extension = null; filename = null; pattern = null; }

Account for that case in mkIf condition so that the option would not
produce empty filetype definition.
@stasjok stasjok force-pushed the no-filetype-by-default branch from a306b93 to 34aa3e0 Compare July 22, 2024 08:39
Copy link
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!


assertions = [
{
assertion = builtins.match ".*vim\.filetype\..*" config.content == null;
Copy link
Member

Choose a reason for hiding this comment

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

BTW you can use lib.hasInfix when you don't actually need a regex:

Suggested change
assertion = builtins.match ".*vim\.filetype\..*" config.content == null;
assertion = lib.hasInfix "vim.filetype." config.content;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I already found out about it yesterday when working on modules/output refactoring. I intended to change it in other PRs, but looks like I forgot about this.

@MattSturgeon MattSturgeon merged commit 34aa3e0 into nix-community:main Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants