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

Cleanup and implications of async_call() usage #28

Open
numirias opened this issue Nov 7, 2018 · 3 comments
Open

Cleanup and implications of async_call() usage #28

numirias opened this issue Nov 7, 2018 · 3 comments

Comments

@numirias
Copy link
Owner

numirias commented Nov 7, 2018

Thanks to #26, async API calls from threads are now implemented correctly. This may resolve some problems and allow some additional cleanups.

  • Maybe proper unit test is required? (not sure)

    @wookayin I don't think so, the current tests should already catch that.

  • Should we prevent creating wrapper function instance every time?

    @wookayin I don't think it has an impact on performance. However, is there a particular reason why you preferred the wrapper function over calling nvim.async_call() directly?

  • Is it still necessary to use call_atomic() in small batches or does Use nvim.async_call in non-main threads #26 remediate that issue?

  • It seems now that that we schedule API calls correctly, there are some interactions with other plugins. E.g., I observed that using Semshi with deoplete now slows it down significantly. Is there something we can do about it?

@wookayin
Copy link
Contributor

wookayin commented Nov 7, 2018

Thanks for your follow-up. For the first two items, I agree with you. Initially I tried with calling like nvm.async_call(fn, arg1, arg2, kwarg=1) but it was a bit confusing to me. Generally speaking the wrapper style seems more intuitive to me as it looks more like calling a callable (function).

For the latter two items, I'm not quite sure. But I would be happy to help with following them up.

@numirias
Copy link
Owner Author

numirias commented Nov 8, 2018

Wrapping the callable makes a lot of sense, but I think I'd prefer lambda expressions because that spares us the extra wrapper function while keeping the code easy to read. E.g. instead of

self._wrap_async(self._buf.clear_highlight)(ERROR_HL_ID)

you'd do

self._vim.async_call(lambda: self._buf.clear_highlight(ERROR_HL_ID))

But either way, it's not a pressing issue.

@wookayin
Copy link
Contributor

wookayin commented Nov 8, 2018

I also like the lambda-style way of calling, as it looks more concise. I would be happy with you cleaning up the codes in such a way.

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

No branches or pull requests

2 participants