-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENHANCE]: Error message for plugin. #4441
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Typocalypse thanks for working on this!
plugin/black.vim
Outdated
finish | ||
endif | ||
|
||
" define commands inly if all checks are passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inly -> only?
plugin/black.vim
Outdated
" define commands inly if all checks are passed | ||
func! BlackNotSupported() | ||
echo "The black.vim plugin cannot be used with this Vim executable." | ||
endfunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this function is defined when both check pass, but it should be define only if any of the checks did not pass.
Also BlackNotSupported
looks like a public name that a user may call from the vim prompt line. The original function has a __
prefix.
How about something like this:
if version is too:
define __BlackNotSupported with version error message
elif python3 suppoort is mising:
define __BlackNotSupported with python3 error message
if __BlackNotSupported is defined:
define Black, BlackUpgrade, BlackVersion functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for you suggestion.
@nirs Is it a good idea to display messages with the current version and the required version in the error message, or is it just unnecessary overhead? |
I don't think that showing vim version is helpful to users. The only useful thing is the required version, in case you run a very old vim. Even if this was useful, vim version is displayed when you start vim and available both via vim --version and using the package manager. |
func! __BlackNotSupported() | ||
echo "The black.vim plugin requires Vim compiled with Python 3.8+ support." | ||
endfunc | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will hide the version requirements if python3 support is not available. I think it will be more useful to use:
if v:version < 700
...
elseif !has('python3')
...
endif
Of course with this if you had very old vim and you get vim 7+ without python3 support you will find about the python3 support on the second try. The current error message avoid this issue by displaying both requirements, but fail to explain what is the actual issue.
We can do something like this:
Vim is too old:
Vim {version} is too old.
The black.vim plugin requires Vim 7.0+ with python3 support.
Python3 is missing:
Python3 support is not available.
The black.vim plugin requires Vim 7.0+ with python3 support.
Description
This enhance the error message in Vim plugin, added a checks for minimum Vim version and Python 3 support in the vim plugin with clear error message.
Checklist - did you ...
CHANGES.md
if necessary?