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

Default missing files to use normal gf (and hence don't force the 'gf' mapping on users. #6

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

Conversation

idearat
Copy link

@idearat idearat commented Mar 21, 2014

I was enjoying the basic functionality of this but found it frustrating that my own path and suffixadds values were ignored. That means files within my project's 'src' directory may not be found etc. if I'm using this plugin. This set of changes fixes that so that 'normal gf' is the fallback. To make that work cleanly I changed from mapping keys to exporting commands that the user can map as they see fit (I ended up with o myself).

@moll
Copy link
Owner

moll commented Mar 22, 2014

Hey, Scott! Thanks for caring and taking the time to do something about it!

Oh, definitely Node.vim should fall back to the default gf implementation. Better yet it should delegate to whatever was there before. But the default would be better than nothing, absolutely! Thanks for that!

A minor style comment on that gf ef14fda commit: The file is indented with tabs, but the else line there seems to be with spaces. :)

Did you get a chance to think about automated tests for that first commit?

I'm not entirely decided on the other commit regarding mappings, but I get where you're coming from. I'd ideally see those built-in mappings augmenting existing functionality (without breaking, of course!) by default with perhaps an opt-out possibility.

On that topic, why did you turn those <Plug> maps to commands? Is that better? :)

Thanks again!

@idearat
Copy link
Author

idearat commented Mar 22, 2014

Hi! Thanks for the quick and thoughtful response.

Re: fallback to 'gf' let me clean up the style nit and think about some tests. Good that you pointed it out.

Re: the key binding, this might be as much a personal preference as anything else. The current code works, although since you map 'gf' inside the plugin itself it made it challenging for me to get a clean fallback mapping to work like I wanted. That's largely the impetus behind my change to commands since it exposes them all as :NodeGotoFile etc. and makes them easy for users to map as they see fit. There are a couple options as you point out like trying to set some global configuration flags etc. but I must admit I'm not a vim-script guru by any means so I may have taken the easy path here :). Let me know if you have other ideas. I'm happy to adjust the pull request into something that works for both of us.

@moll
Copy link
Owner

moll commented Mar 22, 2014

Let's approach those two things separately and get a decent gf going first. I guess we could even see if gf is mapped before overmapping and then fall back to either normal gf or to the previous mapping.

Btw, those <Plug> mappings are global, so they can be remapped. The if line there actually prevents Node.vim's mappings if you map <Plug>NodeGotoFile in your vimrc. So in your case that would be something like:

au User Node nmap <buffer> go <Plug>NodeGotoFile

That would stop all other defaults from being set, too.

@idearat
Copy link
Author

idearat commented Mar 22, 2014

Works for me. If you'd like to abandon this pull request and do a simple update to the gf fallback logic that would meet my needs based on what you've suggested here re: mappings.

@wilmoore
Copy link

+1 to @idearat's last comment.

@pgilad
Copy link

pgilad commented Jun 18, 2014

@moll
I also was frustrated by the lack of the default path support that was disabled by this plugin.
I'm working in a Backbone + Requirejs project where all my AMD defines need to be relative to js/ directory, and I find that by setting set path+=js I easily solve the problem natively, while using vim-node it simply does not work.

So obviously 2 needed workarounds for this (either one would do, but both are valuable):

  1. Don't force map on gf, instead let users decide whether to use default mappings for the plugin, or bind their own
  2. If locating path/file has failed under the bound gf, fallback to the native gf.

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.

4 participants