RRethy / vim-illuminate

illuminate.vim - (Neo)Vim plugin for automatically highlighting other uses of the word under the cursor using either LSP, Tree-sitter, or regex matching.
2.18k stars 50 forks source link

allowlist only works after empty denylist explictly #178

Closed waynerv closed 1 year ago

waynerv commented 1 year ago

After setting the allowlist, found that did not work.

Diving into the source code, found that it is mutually exclusive with the denylist option. In order for the allowlist to take effect, the default value of the denylist must be explicitly cleared:

filetypes_denylist = {},
filetypes_allowlist = { "c", "python" },

There are no logical problems, but I think it can be slightly improved.

https://github.com/RRethy/vim-illuminate/blob/8c910b2f84ae6acd9b4b17330bb94dd783c0c11a/lua/illuminate/util.lua#L22

waynerv commented 1 year ago

what about:

function M.is_allowed(allow_list, deny_list, thing)
    allowed = true
    if #allow_list == 0 and #deny_list == 0 then
        return allowed
    end

    if #allow_list >0 then
        allowed = vim.tbl_contains(allow_list, thing)
    end
    if #deny_list > 0 then
        allowed = not vim.tbl_contains(deny_list, thing)
    end
    return allowed

end

It's more likely "override".

RRethy commented 1 year ago

what about:

function M.is_allowed(allow_list, deny_list, thing)
    allowed = true
    if #allow_list == 0 and #deny_list == 0 then
        return allowed
    end

    if #allow_list >0 then
        allowed = vim.tbl_contains(allow_list, thing)
    end
    if #deny_list > 0 then
        allowed = not vim.tbl_contains(deny_list, thing)
    end
    return allowed

end

It's more likely "override".

This code breaks with the following config filetypes_allowlist = {'foo'}. This should mean the highlighting is only done on filetypes foo, however, it will return true for filetypes bar as well.

The issue is the default values for the denylist. I'm going to document this but not change it since the denylist is the more useful of the two lists IMO so keeping those defaults there is beneficial for more people.