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.

Sign up for GitHub

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

Jump to bottom

properly remove interceptor with regex domain from the list after used #520

Merged
merged 1 commit into from
Apr 4, 2016
Merged

properly remove interceptor with regex domain from the list after used #520

merged 1 commit into from
Apr 4, 2016

Conversation

ericsaboia
Copy link
Contributor

From the issue reported on #508.

The regex domain interceptor wasn't being removed from the list because the method was using protocol host to find the interceptor in the list. Changed to use interceptor.basePath which is reliable for any kind of interceptor.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 92.632% when pulling 2834178 on ericsaboia:master into 3ce48c9 on node-nock:master.

@ericsaboia
Copy link
Contributor Author

How is it possible that this PR is decreasing the coverage? :(

@ericsaboia
Copy link
Contributor Author

and by the way, why do we have removeInterceptor and remove. Should't that be the same method?

@pgte pgte merged commit 9684dc6 into nock:master Apr 4, 2016
@pgte
Copy link
Member

pgte commented Apr 4, 2016

@ericsaboia thanks!

Fix landed on v7.7.3.

@pgte pgte mentioned this pull request Apr 4, 2016
Closed
@pgte
Copy link
Member

pgte commented Apr 4, 2016

@ericsaboia the remove function is what is passed into the RequestOverrider constructor..

@ericsaboia
Copy link
Contributor Author

got it @pgte - but IMO it could be a single method. They were almost duplicated before, and now the removeInterceptor still has the bug that I've fixed for remove.

I can try to refactor that if you want.

@pgte
Copy link
Member

pgte commented Apr 4, 2016

Yes, please! :)

On 4 Apr 2016 15:28 0100, Eric [email protected], wrote:

got it@pgte(https://github.com/pgte)- but IMO it could be a single method. They were almost duplicated before, and now theremoveInterceptorstill has the bug that I've fixed forremove.

I can try to refactor that if you want.

—
You are receiving this because you were mentioned.
Reply to this email directly orview it on GitHub(#520 (comment))

@lock
Copy link

lock bot commented Sep 14, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Sep 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ericsaboia @coveralls @pgte