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

Fetch facebook data from BOT #20

Merged
merged 3 commits into from
Jan 24, 2018
Merged

Conversation

hadasiii
Copy link
Contributor

Updated the stream to not include since when pastdays is undefined,
by so fetching data from BOT.
This is needed for godaddy custom solution.

@hadasiii hadasiii self-assigned this Jan 18, 2018
@hadasiii hadasiii requested a review from oshribin January 18, 2018 08:43
Copy link
Contributor

@oshribin oshribin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hadasiii I have no problem with the changes as they simply omit undefined properties from the request string which is great. But I am not sure I understand what is this BOT, What does it mean fetching data from BOT?
It also should be explained in the README file of this repo. (NOTE that it is public).

I am rejecting because of the testing, I am not sure what you are testing. And I don't think that relying on the stream.url in the test is a good idea, it is fragile. You can use the URL argument from the call to request.getAsync.

test.js Outdated
before( initialize( result, response, source, true ) )
after( reset )

it( 'Skip missing data', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what are you testing here and what it has to do with missing data?

test.js Outdated
before( initialize( result, response, source ) )
after( reset )

it( 'Skip missing data', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, What are you testing?

@hadasiii
Copy link
Contributor Author

hadasiii commented Jan 21, 2018

@oshribin
ive changed the test desc - mistake on my part
the tests checks the built url to see if it considers past days or not.
If it doesn't it means it brings the data from beginning of time .
Added also an update to the README file

@hadasiii hadasiii requested a review from oshribin January 21, 2018 09:46
@hadasiii hadasiii merged commit 05b14f4 into panoplyio:master Jan 24, 2018
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