-
Notifications
You must be signed in to change notification settings - Fork 36
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
Allow GH user to exist without having TODO before it #63
base: main
Are you sure you want to change the base?
Conversation
@@ -5,7 +5,7 @@ local async = require('hover.async') | |||
---@return string | |||
local function get_user() | |||
local WORD = fn.expand('<cWORD>') | |||
local user = WORD:match('TODO%(@?(.*)%):') | |||
local user = WORD:match '%f[%w_]@?(.*)%f[^%w_]' |
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.
What is this expected to match in practice? Some comments would help.
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.
I don't think the regex is perfect, but it matches @user
and user
, the %f[%w_]
and %f[^%w_]
is because of the lack of \b
in Lua. https://stackoverflow.com/a/32854326/213124
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 cause a lot of false-positives as most words exist as a gihutb user.
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.
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.
The plugin depends on an explicit action (moving the cursor on a word and then triggering the hover) so the false-positives would be minimal. But we can make the @
mandatory too, so it matches only @user
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.
The plugin depends on an explicit action (moving the cursor on a word and then triggering the hover) so the false-positives would be minimal.
Maybe that reflects how you use the plugin, but I use this with the dictionary provider and I don't want to be viewing github users as the first result when I want the definition for a word.
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.
Making @
mandatory instead of optional might be a good middle ground then?
I dunno what's the reason for having
TODO(@user)
orTODO(user)
as a requirement to get GH user information, but I don't think that's needed, and making it generic will allow it to work more contexts (includingTODO
). What do you think?