feat: validate all option types

[Akmadan23]

Discussed in #2404

[Akmadan23]

There is this style issue that I predicted, but I think that semantically makes more sense to have the list members on a single line... Even vim.inspect formats tables this way.

@alex-courtis if you're ok with that I'd add a -- stylua: ignore in front of it.

[gegoune]

What is the problem with style formatting here?

[Akmadan23]

What is the problem with style formatting here?

This here: in my opinion keeping the list members on the same line is more consistent with all the other fields and is better for readability.

[gegoune]

I am not disagreeing with you here specifically, but using opinionated formatters such as stylua sometimes requires sacrifices. If we start ignoring chunks of code on a whim we gonna end up with inconsistent looking code. I'd say let's just let style do its job. It might be worse than what you are proposing but I think consistency matters as well.

---

Thanks for that and previous PR! Excellent work!

[Akmadan23]

I am not disagreeing with you here specifically, but using opinionated formatters such as stylua sometimes requires sacrifices. If we start ignoring chunks of code on a whim we gonna end up with inconsistent looking code.

You have a good point, however there is already an exception (and for a good reason) in nvim-tree/keymap.lua, so maybe another little exception here wouldn't be so bad. Seeing all lists on a single line except this one feels a bit awkward to me...

---

By the way, thank you for your appreciation, I'm really happy to contribute and improve the software I use and I'm getting on very well here!

[alex-courtis]

Love your work @Akmadan23

I'll review / test this weekend.

[alex-courtis]

Tested OK with each type:

on_attach
sort.sorter
renderer.root_folder_label
actions.open_file.window_picker.picker

Not checking view.width e.g.
view.width.min = true

Regular type checks not validating e.g.
live_filter.prefix = 1
diagnostics.icon.hint = function() end

We could display more information about the expected types e.g.
[NvimTree] invalid option: on_attach expected: string actual: number | See :help ...
[NvimTree] invalid option: on_attach expected: function|string actual: number | See :help ...

I'm not sure what to do about view.width... playing with some options now.

[alex-courtis]

We could be pragmatic about the validation tables, using full "paths", maybe something like:

  ["on_attach"] = { "function", "string" },
  ["sort.sorter"] = { "function", "string" },
  ["view.width.min"] = { "string", "function", "number" },
  ["view.width.max"] = { "string", "function", "number" },

I seem to recall similar difficulties with the legacy options, which are using a similar pattern.

[Akmadan23]

With this commit I fixed the regular type checks not validating and added more information when expected types are more than one.

The issue regarding view.width.min and view.width.max has to do with this check:

if type(user) ~= "table" or type(def) ~= "table" or ... then
  return
end

Since view.width in the defaults opts is not a table those fields will never be checked. What could we do about that?

[alex-courtis]

Here's a unit test of sorts:

On master:
Call setup with all invalid options: opts_all_bad.lua.gz

:put =execute('messages')
:%s/ | /\r/g
:w err.expected

Repeat on branch as err.actual

diff <(sort err.expected) <(sort err.actual | sed -e 's/\. Expected/ expected:/g ; s/, got/ actual:/g ; s/Invalid/invalid/g')

Everything looks good.

[alex-courtis]

New format works well:

[NvimTree] Invalid option: renderer.root_folder_label. Expected function|string|boolean, got number | see :help nvim-tree-opts for available configuration options

[alex-courtis]

Since view.width in the defaults opts is not a table those fields will never be checked. What could we do about that?

No solution has emerged for me.

I think we'll just need to go with the dotted string option: #2414 (comment)

Implementation is no drama as we have prefix and k to directly compare to.

[Akmadan23]

I think we'll just need to go with the dotted string option

I don't think that would fix the issue, the initial check for default's type would still be there and that is what is causing the problem. However I feel like I can find a solution changing something about that check... When I'll get it to work I'll push it.

[Akmadan23]

I found this solution, which might look a bit clunky, but seems to work well...

[alex-courtis]

Looking good!

Tested OK:

• regression feat: validate all option types #2414 (comment)
• accepted: on_attach, sort.sorter, renderer.root_folder_label, actions.open_file.window_picker.picker
• view.width direct type

No message:

• view.width.min = false

As @gegoune said, sometimes we just have to accept some sacrifices. At some stage we might increase length from 120 to 140 or 160 as it is resulting in some ugly split lines.

• stylua

Exception:

• view.width.foo = "bar"

E5113: Error while calling lua chunk: .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:703: attempt to index local 'def' (a number value)
stack traceback:
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:703: in function 'validate'
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:724: in function 'validate'
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:724: in function 'validate'
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:730: in function 'validate_options'
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:766: in function 'setup'
        /tmp/nvt-dev/nvt-dev.lua:70: in main chunkE5113: Error while calling lua chunk: .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:703: attempt to index local 'def' (a number value)
stack traceback:
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:703: in function 'validate'
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:724: in function 'validate'
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:724: in function 'validate'
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:730: in function 'validate_options'
        .../packer/start/refactor-opts-validation/lua/nvim-tree.lua:766: in function 'setup'
        /tmp/nvt-dev/nvt-dev.lua:70: in main chunk

[alex-courtis]

The errors can get very wordy, with too many [NvimTree].

Here's a patch to trim things a bit:

[NvimTree] Invalid option: prefer_startup_root: expected boolean, actual string | Unknown option: foo | Invalid value for field view.side: 'top' | see :help nvim-tree-opts for available configuration options

trim-validate-error.patch.gz

Let me know what you think.

[Akmadan23]

The errors can get very wordy, with too many [NvimTree].

I completely agree, in fact I was planning to open a PR with exactly the same changes after this one, but putting it in this refactor is even better.

[Akmadan23]

No message: view.width.min = false
Exception: view.width.foo = "bar"

Now I'll address these two issues.

[Akmadan23]

Turns out it was way easier than what I did previously.
I got rid of check_def, which just caused confusion, and assigned an empty table to def so that we can always call def[k] safely.

[alex-courtis]

Turns out it was way easier than what I did previously. I got rid of check_def, which just caused confusion, and assigned an empty table to def so that we can always call def[k] safely.

Great work. I'll test this on the weekend.

[alex-courtis]

Fantastic! All test cases pass!

[alex-courtis]

I've styled for now so we can get this in.

With a width of 140 we get

  view = {
    width = { "string", "function", "number", "table", min = { "string", "function", "number" }, max = { "string", "function", "number" } },
  },

However that's a change for another day.