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

Consider merge to nock, and nock to flatiron. #4

Closed
heapwolf opened this issue Jan 4, 2012 · 11 comments
Closed

Consider merge to nock, and nock to flatiron. #4

heapwolf opened this issue Jan 4, 2012 · 11 comments

Comments

@heapwolf
Copy link

heapwolf commented Jan 4, 2012

Let's consider bringing @pgte's nock (https://github.com/pgte/nock) into flatiron in favor of this project and merging any features from this project into nock.

@indexzero
Copy link
Member

Not opposed to this, although there doesn't seem to be anything in this library worth porting. @pgte I have some reservations about overwriting http.request. Not in-so-far that I disagree with it, but that it's not 100% mocked.

All it takes is one smart-ass library author (or ancient library) to use http.createClient or instantiate http.ClientRequest directly. This circumvents nock.

Would you be open to overriding http.ClientRequest instead?

@pgte
Copy link

pgte commented Jan 5, 2012

@indexzero Yup, no problem, fortunately http.ClientRequest is exposed. I opened this issue: nock/nock#26

@xla
Copy link

xla commented Jan 11, 2012

Only to add some thoughts from the nodejs-dev google group where it's stated that bare constructors will likely go away in the future and which would leave us with factories and the intended interfaces.

@indexzero
Copy link
Member

@goldjunge I don't take away that point from the discussion at all: felixge, mikeal, polotek, and piscisaureus were all for it. That's a pretty solid list of +1s

@pgte
Copy link

pgte commented Feb 6, 2012

@indexzero @hij1nx I recently finished a big refactoring of nock and http.ClientRequest overriding is included. It now contains 91 client tests (awesome 8 contributors).

@heapwolf
Copy link
Author

heapwolf commented Feb 7, 2012

w00t! Let's drop it into flatiron! Just transfer it from the admin page whenever you're ready. And then we can just delete this repo.

@indexzero
Copy link
Member

@pgte @hij1nx Sounds good. We shouldn't straight up delete this repository for a couple of months though. A deprecation warning and redirect to nock will help transition anyone using this.

@heapwolf
Copy link
Author

heapwolf commented Feb 8, 2012

Agreed, closing issue.

@heapwolf heapwolf closed this as completed Feb 8, 2012
@pgte pgte reopened this Feb 13, 2012
@pgte
Copy link

pgte commented Feb 13, 2012

@hij1nx I don't have write access to the flatiron org. How do you want to do this?

@bithavoc
Copy link

How about adding a disclaimer in the readme? "We recommend you to use Nock instead" or something.

@indexzero
Copy link
Member

Added deprecation notice. Closing.

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

5 participants