#1166 validate user's options

[alex-courtis]

closes #1166

Quite easy: validate user's options against DEFAULT_OPTS.

@geoboom I would be very grateful if you can test, test, test.

Edit: apologies @geoboom I meant to ask @gegoune to test this one ;)

[kyazdani42]

I like the implementation :)

[gegoune]

Got unknown option: filters.custom.1 so either this PR needs rebasing or me asking for that feature was not unreasonable after all. :)

Edit: according to readme filters.custom is a table, so it should not warn me about it, should it?

[kyazdani42]

i think the ignore list might be incorrect ?

[gegoune]

In terms of notification itself seems like there is extra empty line.

[gegoune]

i think the ignore list might be incorrect ?

  filters = {
    custom = { '.git', 'node_modules', '.cache', '.DS_Store' },
  },

Nothing really fancy in there I think.

[kyazdani42]

not yours @gegoune , the one in the PR ^^

[alex-courtis]

Got unknown option: filters.custom.1 so either this PR needs rebasing or me asking for that feature was not unreasonable after all. :)

Edit: according to readme filters.custom is a table, so it should not warn me about it, should it?

That is definitely not working correctly. It should go in NO_VALIDATE_OPTS.

However, reporting an itable index is also wrong and needs to be fixed.

[alex-courtis]

In terms of notification itself seems like there is extra empty line.

That can be fixed.

@gegoune how is that message formatted? I've been testing with nvt-min and plain text formatting. Maybe share your config?

[gegoune]

@alex-courtis it's default config for rcarriga/nvim-notify.

[alex-courtis]

Hardened and rewrote:

• removed ignore list; handle empty default tables during validate
• deletes invalid options
• checks all options in each table after a failure
• use one line for message; newlines upset everything
• refactored the legacy migrations / warnings to happen before validation
• system_open.cmd default is now "" to avoid the nil value problem

[kyazdani42]

very nice improvement :) thanks !

[kyazdani42]

@gegoune does this work now for you as expected ?

[gegoune]

@gegoune does this work now for you as expected ?

Yes, it does. Looks good now and detects incorrect keys, at least handful that I tried Thanks!

[alex-courtis]

@gegoune does this work now for you as expected ?

Yes, it does. Looks good now and detects incorrect keys, at least handful that I tried Thanks!

Thanks for the testing @gegoune

I think we can merge now.

[alex-courtis]

There was an extra hide_root_folder in the above table. It doesn't appear to be used, so I deleted it.

[kyazdani42]

good catch, it wasn't in the right place !

[gegoune]

Thanks for that! Hopefully it will prevent configuration-issue issues.

[eeeXun]

I set up width with function. Not working anymore?

require("nvim-tree").setup({
    view = {
        width = function()
            return math.floor(vim.o.columns * 0.225)
        end,
    }
})

Warning

[NvimTree] invalid option: view.width expected: number actual: function

[alex-courtis]

[NvimTree] invalid option: view.width expected: number actual: function

Interesting use case: functions for configuration options is something I would never have considered.
+ great user flexibility
- poor stability
- unpredictable behaviour when a varying value is used

Alternatives:

1. No change: do not allow functions
2. Allow a function when the type is not a table
3. Allow a function always
4. Provide a proper mechanism for this, something like view.width_ratio

I like flexibility; 2 is a good compromise. We would need to document that with warnings and a rule that bugs cannot be raised when such functions are used. @kyazdani42 ?

Edit: I have implemented 3: #1195 It might be useful to allow functions as tables for, say, diagnostics.icons

[gegoune]

How about 'allow functions but require returned value of specific type and validate it'?

[alex-courtis]

How about 'allow functions but require returned value of specific type and validate it'?

That would be great, have you got a specific idea on how we would do this?

The user could do anything in that function; I don't think we could validate it by executing it, as it may throw an exception when run at plugin startup.

[gegoune]

Ah, that's a good point. Can't really think of a solution here.

[kyazdani42]

there is only a function available for the tree width / height but the implementation for the validation is good enough :)

[eeeXun]

Maybe provide an option like relative_width/relative_height. If true, then the number of width/height should be percentage of columns/lines. If false, the it should be actual columns/lines.

[kyazdani42]

you can pass in '40%' and it will adapt to the number of columns.