nvim-neorocks / rocks-config.nvim

Allow rocks.nvim to help configure your plugins.
GNU General Public License v3.0
55 stars 2 forks source link

fix: avoid loading configs twice #30

Closed tarcisioe closed 4 months ago

tarcisioe commented 4 months ago

Fixes possible points where the plugin can load the same configuration twice.

github-actions[bot] commented 4 months ago

Review Checklist

Does this PR follow the Contribution Guidelines? Following is a partial checklist:

Proper conventional commit scoping:

If applicable:

tarcisioe commented 4 months ago

This fixes the issue of loading configurations twice. There are three things to notice:

First, create_plugin_heuristics can create duplicate items. I added a dedup function to avoid even trying to load the same file twice. This change can be discarded since it is redundant with the rest, but I guess not trying the same thing twice can be less confusing if someone is stepping through the code.

Second, try_load_config checks for package.loaded[mod_name] to be true (actually not nil or false). However, since this code skips the require mechanism it wasn't doing something that require actually does: if the loaded module returns nil (which most configuration modules will, actually), require populates package.loaded with true. Since the code was using the result of loader(), nil was being added, therefore the check failed the next time. Modules can in fact return false as well, so I changed the check for an explicit nil check.

Third, the lookup for a configuration module didn't stop if a valid configuration was found, therefore it was possible for a user to have two configuration files for the same plugin. This is opinionated on my part, but I believe that shouldn't happen. In fact, it would even be possible to warn the user that a given plugin has two configuration files, and I would be open to implementing that as well if desired.

Closes #29.

mrcjkb commented 4 months ago

Third, the lookup for a configuration module didn't stop if a valid configuration was found, therefore it was possible for a user to have two configuration files for the same plugin. This is opinionated on my part, but I believe that shouldn't happen. In fact, it would even be possible to warn the user that a given plugin has two configuration files, and I would be open to implementing that as well if desired.

@vhyrro what do you think about this? Personally, I'm fine with it.

mrcjkb commented 4 months ago

Looks like stylua is failing:

       > +++ b/lua/rocks-config/init.lua
       > @@ -42,7 +42,7 @@ local function try_load_like_require(searcher, mod_name)
       >      local loader = searcher(mod_name)
       >  
       >      if type(loader) ~= "function" then
       > -       return nil
       > +        return nil
       >      end
       >  
       >      local module = loader()
vhyrro commented 4 months ago

Third, the lookup for a configuration module didn't stop if a valid configuration was found, therefore it was possible for a user to have two configuration files for the same plugin. This is opinionated on my part, but I believe that shouldn't happen. In fact, it would even be possible to warn the user that a given plugin has two configuration files, and I would be open to implementing that as well if desired.

@vhyrro what do you think about this? Personally, I'm fine with it.

Yes I agree that having two different config files for the same plugin is in 90% of cases user error. We shouldn't allow that to happen, so I'm happy to warn when dupes are found :)

tarcisioe commented 4 months ago

Fixed typing and styles. @vhyrro I'll give the detection of duplicates with a warning a go on a separate PR because I'll need to give it a bit more thought about how I can detect it without actually attempting to load the second file.