-
Notifications
You must be signed in to change notification settings - Fork 234
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
third party noop library #92
base: master
Are you sure you want to change the base?
Conversation
…my own noop (you jsut don't do that)
Awesome, looks like tests passed. |
Hey @grantholly if you weren't aware already, you haven't uploaded your PGP key to GitHub yet. |
@@ -1,7 +1,7 @@ | |||
'use strict' | |||
|
|||
var isCI = require('is-ci') | |||
var noop = function () {} | |||
var noop = require('node-noop').noop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you write any unit tests for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't write any new tests.
what the hell did I just read |
At first look, I was very sceptic about this change. But I appreciate this, after a closer look at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
You should use the |
Following software design best practices I'm not going to be rolling my own noop (you jsut don't do that)
This addresses #90