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

Fix RestStopRequest event #223

Merged
merged 2 commits into from
Sep 25, 2023
Merged

Fix RestStopRequest event #223

merged 2 commits into from
Sep 25, 2023

Conversation

ddomurad
Copy link
Contributor

Hello :)

Events implementation as proposed in 214 seems to work incorrectly.

Issue 1:
Because a callback function is passed to plenary's curl, the request will be made asynchronously, causing the RestStopRequest event to be emitted right after the local 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 the last request.

Proposed changes:

  1. Move the emition of events into the curl/init.lua file. This way it won't matter if the call is made via run_request or last method. I'm actually not sure about his one though. Maybe it would be better to extract the events back to the main init.lua file, however it was tempting how easy it was to implement.
  2. Emit the RestStopRequest inside the curl callback function.
  3. Add a on_error handler for plenary's curl. This allows us to emit the RestStopRequest event, when the request fails (.e.g. could not resolve the URL)

The proposed changes will alter:

  1. The payload of RestStartRequest and RestStopRequest - 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.
  2. The call stack of the emited error while calling error(err.message) in create_error_handler

Ofc. 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 :(

@ddomurad ddomurad changed the title Ddomurad/refactor events Fix RestStopRequest event Aug 13, 2023
@teto
Copy link
Collaborator

teto commented Aug 23, 2023

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.

@teto
Copy link
Collaborator

teto commented Sep 12, 2023

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 ?

@ddomurad
Copy link
Contributor Author

ddomurad commented Sep 15, 2023

I will try to find some time this weekend, and look into how to write tests for this.

@ddomurad
Copy link
Contributor Author

@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.

@teto
Copy link
Collaborator

teto commented Sep 25, 2023

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 nvim -l)

@teto teto merged commit 16c1c8d into rest-nvim:main Sep 25, 2023
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