Fix RestStopRequest event #223
Fix RestStopRequest event #223teto merged 2 commits intorest-nvim:mainfrom ddomurad:ddomurad/refactor_events
Conversation
|
thanks for looking into it, I might have been a bit cow-boy about the merge (we need more tests :) ). I will have a look in upcoming days when I am able to use rest.nvim again. |
|
That looks much better than my initial proposition. I haven't really tested. Do you think you could plug some test ? if not do you think you could do it later once the test infra improves ? |
|
I will try to find some time this weekend, and look into how to write tests for this. |
|
@teto Well, looking at the current tests implementation, I don't think I will be able to add proper testing for my changes. However I will be glad to help when the proper test infrastructure is there. |
|
cool. I haven't had time to work on rest.nvim recently, if you are interested in improving tests, you can have a look/resume #225 , basically it tries to use busted using nvim as a a lua interpreter (via |
Hello :)
Events implementation as proposed in 214 seems to work incorrectly.
Issue 1:
Because a
callbackfunction is passed to plenary's curl, the request will be made asynchronously, causing theRestStopRequestevent to be emitted right after thelocal success_req, req_err = pcall(curl.curl_cmd, Opts)returns, way before the actual request finishes.Issue 2:
The events are emitted only during normal
run_request, they are not emitted during rerunning thelastrequest.Proposed changes:
curl/init.luafile. This way it won't matter if the call is made viarun_requestorlastmethod. I'm actually not sure about his one though. Maybe it would be better to extract the events back to the maininit.luafile, however it was tempting how easy it was to implement.RestStopRequestinside the curl callback function.on_errorhandler for plenary's curl. This allows us to emit theRestStopRequestevent, when the request fails (.e.g. could not resolve the URL)The proposed changes will alter:
RestStartRequestandRestStopRequest- this can be some what mitigated I guess. However might be not a big deal, if the feature didn't worked correctly in the first place.error(err.message)increate_error_handlerOfc. I'm open to any suggestions :)
I didn't had the time to check if this is in line with other PR that are currently open :(