rcarriga / nvim-notify

A fancy, configurable, notification manager for NeoVim
MIT License
3.08k stars 80 forks source link

notify() passed log level not necesarily valid #97

Closed Frydac closed 2 years ago

Frydac commented 2 years ago

Hi,

I used as log level the string "warning" for some time, but after updating neovim (0.7 -> nightly and a PackerSync since I don't know when) apparently it should be "warn". Maybe it was always wrong and it never notified as an actual warning (and I never noticed), or the string changed, not sure, however I now get an error that on the following line: https://github.com/rcarriga/nvim-notify/blob/master/lua/notify/init.lua#L125 level_num is nil and the comparison on that line fails with an error. I guess this code expects valid input for the level passed to the notify() function, it seems to me it would be nice if this user input was validated before using it, with a more descriptive error/warning msg when it fails.

Also, I think it could be helpful if it is documented better that the level expected to be passed to notify() is related to vim.log.levels (or it used to be vim.lsp.log_leves I guess). I didn't know vim.log.levels/vim.lsp.log_leves existed, and I would think many users don't. There is one example in the main readme.md that uses vim.lsp.log_levels.WARN, but in the doc/nvim-notify.txt the reader more or less has to guess what the levels are referring to afaik.

Probably better practice to just always use vim.log.level.WARN and others, as it seems to me that the string representation is more an implementation detail (and subject to change (even more that the api :) ).

rcarriga commented 2 years ago

On the first part, nvim-notify allows for custom levels so it can't validate the passed level. The error you saw should not have been raised and is now fixed.

For documentation, that makes sense :sweat_smile: Easy to forget people aren't as embedded in the NeoVim ecosystem.... I've updated the docs to refer to vim.log.levels to be clearer