-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add Pingback feature #38
Conversation
lib/pingback.js
Outdated
'result': data.result, | ||
'action': data.action, | ||
'url': data.url, | ||
'source': 'algolia-crawler' |
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.
Please do not quote keys
app.js
Outdated
console.log(data.message); | ||
} | ||
} | ||
}); |
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 do not think we need to output anything here. Simply append to the errors array
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.
@PascalPiche ^^
lib/pingback.js
Outdated
|
||
res.setEncoding('utf8'); | ||
|
||
console.log(res.statusCode); |
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.
Please remove!
lib/pingback.js
Outdated
res.setEncoding('utf8'); | ||
|
||
console.log(res.statusCode); | ||
process.exit(); |
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.
Please remove !
lib/pingback.js
Outdated
console.log(res.statusCode); | ||
process.exit(); | ||
|
||
if (res.statusCode === 404) { |
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.
We do not need to handle 404 differently that if it is non-200
app.js
Outdated
if (pingback.ok == false) { | ||
console.log('No ping back url configured or bad url.'); | ||
console.log(pingback.error); | ||
console.log(); |
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 do not thing we need this error, since no pingback is ok.
README.md
Outdated
result=[success|error] | ||
action=[update|delete] | ||
url=the url inserted | ||
source=”algolia-crawler” |
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.
Can you remove the smart quotes ?
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.
done
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 still see them...
app.js
Outdated
console.log(data.message); | ||
} | ||
} | ||
}); |
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.
@PascalPiche ^^
Should we include a unit test too ? |
README.md
Outdated
result=[success|error] | ||
action=[update|delete] | ||
url=the url inserted | ||
source=”algolia-crawler” |
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 still see them...
app.js
Outdated
|
||
//Add empty line | ||
console.log(); | ||
|
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.
We can remove that empty line
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.
removed
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.
Quote removed from readme.md
app.js
Outdated
result: 'success', | ||
action: 'delete', | ||
url: url.url | ||
}, tearDown); |
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.
Why not pass the callback in the data object ?
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.
And why wait for the pingback to complete ?
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.
To keep console log in sync.
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.
ah ok
lib/pingback.js
Outdated
err: err | ||
}); | ||
}; | ||
req.on('error', (e) => callback(e)); |
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.
There are too many functions here. Either pass callback directly to error, or inline it. I think that the inline is the best.
@fhamon Pretty hard to unit test a http request... |
@nitriques This could be usefull ? https://github.com/nock/nock |
@fhamon yes it would! but mocking is a little bit like voodoo. |
No description provided.