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

Port URL regex to vimscript #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jwhitley
Copy link

This change ports John Gruber's URL regex to Vim's regex format and removes the ruby dependency.

@henrik
Copy link
Owner

henrik commented Apr 30, 2013

Thank you! Looking through the pull request; will drop some comments as I go.

Out of curiosity, is the map to noremap just because it's generally better practise (I agree; I probably didn't know when I wrote this originally)? I can see how it makes a practical difference when mapping to built-in commands (noremap x j), but I'm not sure if there is a practical difference when you're mapping to a custom Ex command.

@henrik
Copy link
Owner

henrik commented Apr 30, 2013

Re: "deferring the question of whether this plugin should map at all", I suppose you mean this. Good point; I should fix that.

@henrik
Copy link
Owner

henrik commented Apr 30, 2013

Hm, actually, I see there is already a if !hasmapto('OpenURL') guard. So if you do a custom mapping, the default one won't be applied. Seems sensible to me; what would you change?

@henrik
Copy link
Owner

henrik commented Apr 30, 2013

Script fails: a missing on line 17 and the wrong method name on line 13.

Even with those fixed, nothing happens. Could you make sure it works for you and reopen the issue after that?

Had it run, I would also have verified that it handles multiple URLs on the same line, since it looks from the code like it might not.

I think this modified plugin doesn't provide any feedback if it finds no matching URL on the current line, either. I suppose I could do without that if it's very hard to achieve.

All this notwithstanding, I'm very impressed you ported the regexp. I'm sure it was hard and painful work :)

@henrik henrik closed this Apr 30, 2013
@jwhitley
Copy link
Author

Hi @henrik, thanks for the review.

Regarding the if !hasmapto('OpenURL'), that isn't quite the issue. It's more that this plugin can clobber a user's existing map to <leader>u. For example, that's where I've mapped in gundo.vim.

I've been using my branch and haven't seen the missing method, so I'm a bit confused as to the problems you're seeing re: wrong method names etc. I may have done something daft like not submitted an up-to-date branch? AAAARGH. I typo'ed something when adding my branch into my local bundles dir, and the local john/vimscript-port ended up on master, not the upstream branch. I.e. I wasn't testing what I thought I was. Apologies; I'll sort that out and update ASAP. I'll also look into providing feedback when no URL is found.

I also noted after submitting the PR that the original code specifies behavior for multiple URLs on the same line. My branch definitely has no special handling for this case; it should just grab the first URL on a line. I'll review the original behavior and see what it would take to preserve it in this port.

@henrik
Copy link
Owner

henrik commented Apr 30, 2013

@jwhitley Sounds great. Thanks so much for taking the time!

@jwhitley
Copy link
Author

jwhitley commented May 1, 2013

@henrik I've updated my branch with a now-working version that restores proper user feedback, but which does not yet handle multiple URLs. I'd like to get vspec tests in using the DF test set in place as well.

FYI, I don't seem to be able to reopen this PR, and it isn't showing my most recent updates while closed.

@henrik
Copy link
Owner

henrik commented May 1, 2013

Oh, suppose only I can reopen. Done! Will hopefully look at the updates later today.

Tests would be very cool. Great problem for it.

@henrik henrik reopened this May 1, 2013
@raine
Copy link

raine commented Jun 8, 2013

FYI, gx seems to do what this plugins does.

@henrik
Copy link
Owner

henrik commented Aug 10, 2013

@raneksi Ooh, interesting. Not quite the same thing: gx doesn't seem to open multiple URLs on the same line. But certainly close. Will have to look into that further.

@jwhitley So sorry I've been ignoring this. Hope to find the time and inclination later during this vacation, but no promises I'm afraid.

@henrik henrik mentioned this pull request Nov 27, 2013
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.

3 participants