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

Require g:rspec_runner explicitly #94

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

Conversation

gylaz
Copy link
Contributor

@gylaz gylaz commented May 22, 2015

If g:rspec_runner is not provided
the g:rspec_command will be piped to GUI vim as-is.

This allows users to use vim-dispatch with GUI vim.

@gylaz gylaz force-pushed the gl-require-gui-runner branch 3 times, most recently from 2380a5f to c6aa1b9 Compare May 23, 2015 02:15
Greg Lazarev added 2 commits May 22, 2015 19:19
If `g:rspec_runner` is not provided
the `g:rspec_command` will be piped to GUI vim as-is.

This allows users to use `vim-dispatch` with GUI vim.
@gylaz
Copy link
Contributor Author

gylaz commented May 23, 2015

ping @christoomey @jferris

@gylaz
Copy link
Contributor Author

gylaz commented May 23, 2015

The only problem I see here is that out-of-the box, this doesn't work for MacVim because the default spec command won't work without !. Could we maybe provide a default runner only when a custom command and a custom runner aren't provided? Any other ideas?


You can set `g:rspec_runner` to anything you want,
provided you include the appropriate script
inside the plugin's `bin/` directory.

Choose a reason for hiding this comment

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

Would it be possible for this to point to something more in the users control? Perhaps a script in ~/bin? They might need to hard code the path, and I believe the script execution in the plugin would need to be updated, but I think that could be a better approach. Otherwise they would need to either gitignore their script, or rebase a branch, or otherwise manage it as an addition to the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting idea. It think this would be a good approach, with allowing people to override the default bin/ path. However, there's no evidence that people are even using this feature. Thus, it would be low priority to focus on something, that's not a reported problem.

@christoomey
Copy link

Overall I think the explicitness requirement is fine, and the readme updates seems good to cover the change. I did have one concern about recommending they add their custom script to the plugin bin/ directory which I commented on inline.

@gylaz
Copy link
Contributor Author

gylaz commented May 26, 2015

Thanks, @christoomey.

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