modules/output: produce empty files by default #1909

stasjok · 2024-07-21T09:40:51Z

This PR removes empty

vim.cmd([[

]])

block from config.

I know that this is already handled by #1889, but I've added tests to make sure that this is the behavior. Also I added the test to make sure that all extraConfigs are outputted.

This also changes various places in other modules with the intention of not setting extraConfig* options to empty strings (more specifically whitespace only strings).

NOTE: The tests will fail until #1906, #1907 and #1908 are merged.

MattSturgeon · 2024-07-21T10:02:09Z

Thanks! I'd already looked at doing this in #1889, however since that one is larger in scope and unfinished it's better to have this merged sooner than later!

It's also good to have tests, which I hadn't implemented in #1889.

Would you mind looking over how I'd done it in that other PR to see if there's any elements you'd like to incorporate in this one?

E.g. it probably makes sense to move hasContent into our helpers lib and maybe come up with some kinda wrapper function to reduce boilerplate when wrapping potentially empty lua/vim strings?

Not too important as those things may expand the size/scope of this PR unnecessarily making it harder to review. But worth a quick look.

stasjok · 2024-07-21T10:10:09Z

Would you mind looking over how I'd done it in that other PR to see if there's any elements you'd like to incorporate in this one?

Your implementation is better, because it doesn't output empty lines. I like it more. But I intentionally didn't rip it off, because I wanted something simple to get started (and functional) + tests. And since you already implemented a better version, I thought that I don't need to make it perfect, just functional, and you'll easily rebase to it later. But if you think that it's better to include your version here, I probably can extract it from there.

MattSturgeon · 2024-07-21T10:27:33Z

But if you think that it's better to include your version here, I probably can extract it from there.

I don't mind too much either way. There's certainly value in keeping this PR simple as you said.

MattSturgeon

(Just reviewing the treewide mkIf commit)

This would be better off in a dedicated PR, IMO.
I don't see how it relates to this one?
There's a typo in the commit message (treeside -> treewide)
Some stuff is either deliberately not using mkIf or is less readable, especially string concatenation.

lib/neovim-plugin.nix

modules/autocmd.nix

modules/highlights.nix

modules/opts.nix

modules/top-level/output.nix

stasjok · 2024-07-21T18:49:10Z

I don't mind too much either way. There's certainly value in keeping this PR simple as you said.

I rewrote it completely. See if it's OK.

Also I replaced a couple of global instances of optionalString with mkIf. It's in a separate commit, I can drop it if needed.

Before:

nix-repl> c.options.extraConfigLuaPre.definitions      
[ "\n\n\n\n" "" "" "-- Ignore the user lua configuration\nvim.opt.runtimepath:remove(vim.fn.stdpath('config'))         
     -- ~/.config/nvim\nvim.opt.runtimepath:remove(vim.fn.stdpath('config') .. \"/after\")  -- ~/.config/nvim/after\nvi
m.opt.runtimepath:remove(vim.fn.stdpath('data') .. \"/site\")     -- ~/.local/share/nvim/site\n" ]

nix-repl> c.options.extraConfigLuaPost.definitions
[ "" "" ]

After:

nix-repl> c.options.extraConfigLuaPre.definitions  
[ "-- Ignore the user lua configuration\nvim.opt.runtimepath:remove(vim.fn.stdpath('config'))              -- ~/.config
/nvim\nvim.opt.runtimepath:remove(vim.fn.stdpath('config') .. \"/after\")  -- ~/.config/nvim/after\nvim.opt.runtimepath
:remove(vim.fn.stdpath('data') .. \"/site\")     -- ~/.local/share/nvim/site\n" ]

nix-repl> c.options.extraConfigLuaPost.definitions
[ "" ]

Notice that there is no empty definitions by default.

But similar cases with empty definitions are still present in some of the plugins, although they are guarded by mkIf cfg.enable, if plugin is enabled, they appear. But I definitely not going to edit them now.

MattSturgeon

(This review is for the other (non-mkIf) changes)

Looks good, mostly just minor nits!

lib/utils.nix

modules/output.nix

modules/top-level/output.nix

tests/test-sources/modules/output.nix

stasjok · 2024-07-21T19:04:58Z

I'll check comments tomorrow. Please decide if a second commit removing empty lines from outputs in a various places is wanted or not.

MattSturgeon · 2024-07-21T19:22:50Z

This would be better off in a dedicated PR, IMO.
I don't see how it relates to this one?

After reading your replies I agree it makes sense in this PR, but it would benefit from a commit message that explains the motivation behind the change, as I was clearly confused 😉

Please decide if a second commit removing empty lines from outputs in a various places is wanted or not.

I'm happy to keep it, though I think empty lines in config that is defined anyway aren't as bad as non-empty lines relating to unconfigured stuff.

stasjok · 2024-07-22T12:12:55Z

Since other PRs are merged, tests are passing.

tests/test-sources/modules/output.nix

modules/opts.nix

MattSturgeon

LGTM, thanks for your efforts and your patience explaining things to me and debating various pros/cons!

I think this change will be appreciated by many!

@nix-community/nixvim any final suggestions before merging?

EDIT: tests all pass locally on x86_64-linux.

GaetanLepage

LGTM after a rapid glance.
I trust @MattSturgeon for the details.

Thank you very much @stasjok for this great work and for addressing the detailed review !

GaetanLepage · 2024-07-22T21:04:00Z

@Mergifyio queue

mergify · 2024-07-22T21:04:10Z

queue

🛑 The pull request has been merged manually

The pull request has been merged manually at 9317537

The motivation for this change was to avoid generating empty config sections like vim.cmd([[ ]]) To make a config generation cleaner several helper functions introduced: * `hasContent` have been moved to helpers * `concatNonEmptyLines` joins strings (which has content) separated with newlines * `wrapVimscriptForLua` wraps a lua string for using in Vimscript, but only if the string has content, otherwise empty string is returned * `wrapLuaForVimscript` wraps Vimscript for using in lua, but only if the string has content, otherwise empty string is returned Added tests: * testing that all possible config sections are present in the final generated config * testing that the config files generated by empty `files` definitions don't have any content in it

Problem: Some modules are setting empty strings to extraConfig* options with the intention to not generate any config. But empty strings are also values, so they are still concatenated in the final value of extraConfig* options. This results in a multiple empty strings in extraConfigs. Solution: Avoid using optionalString when setting values to extraConfig* options. Use mkIf instead. This commit also fixes mkIf condition in autocmd module. `mkNeovimPlugin` is a special case. To avoid evaluating caller's arguments mkMerge/optionalAttrs pattern is used instead.

stasjok force-pushed the no-empty-cmd-blocks branch from dc71fb6 to d1e2ecf Compare July 21, 2024 10:16

stasjok force-pushed the no-empty-cmd-blocks branch 2 times, most recently from 73c34ca to 13b3fc3 Compare July 21, 2024 18:46

MattSturgeon requested changes Jul 21, 2024

View reviewed changes

lib/neovim-plugin.nix Outdated Show resolved Hide resolved

modules/autocmd.nix Show resolved Hide resolved

modules/highlights.nix Show resolved Hide resolved

modules/opts.nix Outdated Show resolved Hide resolved

modules/top-level/output.nix Show resolved Hide resolved

MattSturgeon requested changes Jul 21, 2024

View reviewed changes

stasjok force-pushed the no-empty-cmd-blocks branch from 13b3fc3 to 9430227 Compare July 22, 2024 12:03

MattSturgeon reviewed Jul 22, 2024

View reviewed changes

tests/test-sources/modules/output.nix Outdated Show resolved Hide resolved

stasjok force-pushed the no-empty-cmd-blocks branch from 9430227 to 5329e98 Compare July 22, 2024 16:58

MattSturgeon reviewed Jul 22, 2024

View reviewed changes

modules/opts.nix Outdated Show resolved Hide resolved

MattSturgeon approved these changes Jul 22, 2024

View reviewed changes

stasjok force-pushed the no-empty-cmd-blocks branch from 5329e98 to d4f37e5 Compare July 22, 2024 17:41

GaetanLepage approved these changes Jul 22, 2024

View reviewed changes

stasjok added 2 commits July 22, 2024 21:04

GaetanLepage force-pushed the no-empty-cmd-blocks branch from d4f37e5 to 0f3edf0 Compare July 22, 2024 21:04

GaetanLepage merged commit 9317537 into nix-community:main Jul 22, 2024
1 check passed

MattSturgeon mentioned this pull request Aug 7, 2024

plugins/which-key: migrate to mkNeovimPlugin #1990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modules/output: produce empty files by default #1909

modules/output: produce empty files by default #1909

stasjok commented Jul 21, 2024 •

edited

Loading

MattSturgeon commented Jul 21, 2024

stasjok commented Jul 21, 2024 •

edited

Loading

MattSturgeon commented Jul 21, 2024

MattSturgeon left a comment

stasjok commented Jul 21, 2024

MattSturgeon left a comment

stasjok commented Jul 21, 2024

MattSturgeon commented Jul 21, 2024 •

edited

Loading

stasjok commented Jul 22, 2024

MattSturgeon left a comment •

edited

Loading

GaetanLepage left a comment

GaetanLepage commented Jul 22, 2024

mergify bot commented Jul 22, 2024 •

edited

Loading

modules/output: produce empty files by default #1909

modules/output: produce empty files by default #1909

Conversation

stasjok commented Jul 21, 2024 • edited Loading

MattSturgeon commented Jul 21, 2024

stasjok commented Jul 21, 2024 • edited Loading

MattSturgeon commented Jul 21, 2024

MattSturgeon left a comment

Choose a reason for hiding this comment

stasjok commented Jul 21, 2024

MattSturgeon left a comment

Choose a reason for hiding this comment

stasjok commented Jul 21, 2024

MattSturgeon commented Jul 21, 2024 • edited Loading

stasjok commented Jul 22, 2024

MattSturgeon left a comment • edited Loading

Choose a reason for hiding this comment

GaetanLepage left a comment

Choose a reason for hiding this comment

GaetanLepage commented Jul 22, 2024

mergify bot commented Jul 22, 2024 • edited Loading

🛑 The pull request has been merged manually

stasjok commented Jul 21, 2024 •

edited

Loading

stasjok commented Jul 21, 2024 •

edited

Loading

MattSturgeon commented Jul 21, 2024 •

edited

Loading

MattSturgeon left a comment •

edited

Loading

mergify bot commented Jul 22, 2024 •

edited

Loading