Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ahmedelgabri
Copy link

I dunno what's the reason for having TODO(@user) or TODO(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 (including TODO). What do you think?

@@ -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_]'
Copy link
Owner

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.

Copy link
Author

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

Copy link
Owner

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

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

Copy link
Owner

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.

Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants