search

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

feat: detect duplicate configs and warn user #33

Closed tarcisioe closed 4 months ago

tarcisioe commented 4 months ago

This pull request is an attempt at duplicate configuration detection and warning as I suggested in #30.

This uses vim.notify with a level of WARN. I thought about implementing this as a health check, but the only options I could think of were:

I'd prefer the first one, and I don't find it too terrible since I don't think it would ever make sense to run this plugin twice. However it would be a bigger change, involves some global state, and I'd like some feedback on the idea before going down that route.

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

Regarding the health check idea: I think the first option makes more sense. But since we're actively warning the user on startup, maybe it's overkill to add a health check?

Absolutely agree. If doing it as a health check is desirable I'd remove the notify and leave everything on the health module.

tarcisioe commented 4 months ago

I added a commit dropping vim.notify and doing the check as a health check. I like it, works nicely. Can keep it if you guys like it, or drop it if you think notify is a better option.

mrcjkb commented 4 months ago

I added a commit dropping vim.notify and doing the check as a health check. I like it, works nicely. Can keep it if you guys like it, or drop it if you think notify is a better option.

Personally, I prefer the startup warning over the health check. But I'll leave this up to @vhyrro.

tarcisioe commented 4 months ago

I was experimenting with adding errors while loading plugin configurations to health.check() and thought about a single notification like "Issues found while loading plugin configs. Run :checkhealth rocks-config for more info.".

vhyrro commented 4 months ago

I was experimenting with adding errors while loading plugin configurations to health.check() and thought about a single notification like "Issues found while loading plugin configs. Run :checkhealth rocks-config for more info.".

I think this is the best solution. At startup we only want to check if something is wrong, not what is wrong, as detailed checks could slightly hinder startup time. A general check that then redirects you to the healthcheck (which can take as much time as you want in the world) is the best of both worlds.

tarcisioe commented 4 months ago

At startup we only want to check if something is wrong, not what is wrong, as detailed checks could slightly hinder startup time.

I agree with that as a general statement, but...

A general check that then redirects you to the healthcheck (which can take as much time as you want in the world) is the best of both worlds.

I don't think in this specific case there is much work separation. Detecting if there is a duplicate config file is basically the same work as detecting which one it is, since we won't load it anyway. In this case I see doing it as a notification or on health check as mostly a presentation issue. However I also think that a single, general notification at startup and more detailed information on healthcheck is less overwhelming (even less annoying) to the user. Having no notification means the user may not ever notice the check, but having detailed information about the error on startup, especially when there may be a few (let's say there is duplicate configuration for two, maybe even three plugins) starts adding up on notifications.

In summary: I think we agree on the same approach, but I'd really like to point out that most of the work will be done at startup anyway (it's not really much more work than was being done before this PR).

vhyrro commented 4 months ago

Right, makes sense. I'm happy to go with that idea either way :)

tarcisioe commented 4 months ago

There it is. Also did a slight reword on the message that was added to checkhealth because on rereading it I found it somewhat confusing.

tarcisioe commented 4 months ago

Hey, sorry to bother, is anything still missing for this to be merged? I have a following PR that I wrote on top of this and it would be easier to point it back to master if this code is already there :grimacing:

mrcjkb commented 4 months ago

Nothing missing from my side :smile: @vhyrro do you want to do the honours?

vhyrro commented 4 months ago

Yessir. Thank you for the PR!