Skip to content

Allow GH user to exist without having TODO before it#63

Open
ahmedelgabri wants to merge 2 commits intolewis6991:mainfrom
ahmedelgabri:patch-1
Open

Allow GH user to exist without having TODO before it#63
ahmedelgabri wants to merge 2 commits intolewis6991:mainfrom
ahmedelgabri:patch-1

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?

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