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

WIP: Refactoring & Fix issue on nvim #101

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

Conversation

lambdalisue
Copy link

@lambdalisue lambdalisue commented Jul 23, 2017

@lambdalisue lambdalisue changed the title Refactoring & Fix issue on nvim WIP: Refactoring & Fix issue on nvim Jul 23, 2017
@lambdalisue
Copy link
Author

I'll fix tests later.

Btw, the behavior was tested only in macOS Sierra so it should be tested in other platform as well.

@@ -523,21 +520,17 @@ function! openbrowser#__open_browser__(uristr) "{{{
return 0
endfunction "}}}

function! openbrowser#__system__(...)
Copy link
Owner

@tyru tyru Jul 23, 2017

Choose a reason for hiding this comment

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

Instead of removing this function, please use s:Process.execute() in this function.
It should also pass failed tests.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@tyru
Copy link
Owner

tyru commented Jul 25, 2017

@lambdalisue Does this PR fix #102 too?

@tyru
Copy link
Owner

tyru commented Jul 28, 2017

ping

@lambdalisue
Copy link
Author

Yes but to support neovim correctly, it should use System.Process.Job internally.

@tyru
Copy link
Owner

tyru commented Jul 28, 2017

@lambdalisue hm, like this? (I don't know if System.Process and System.Process.Job is compatible...)

-let s:Process = vital#openbrowser#import('System.Process')
+let s:Process = vital#openbrowser#import('System.Process.Job')

@lambdalisue
Copy link
Author

We have to wait vim-jp/vital.vim#513
After that, we can use clients options like https://github.com/vim-jp/vital.vim/blob/master/doc/vital/System/Process.txt#L54-L64

@tyru
Copy link
Owner

tyru commented Aug 3, 2017

@lambdalisue Okay. I made a workaround about this in #104.
I'm looking forward to your fix :)

@lambdalisue
Copy link
Author

I'm looking forward to your fix :)

So would you want to wait System.Process.Job?

@tyru
Copy link
Owner

tyru commented Aug 4, 2017

Yes.
To fix this issue completely, vital.vim must be fixed, I think. isn't it?

@lambdalisue
Copy link
Author

Ok!

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.

None yet

2 participants