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

feat: ny daily news parser #87

Merged
merged 3 commits into from
Feb 2, 2017
Merged

feat: ny daily news parser #87

merged 3 commits into from
Feb 2, 2017

Conversation

janetleekim
Copy link
Contributor

The only issue is date formatting:
AssertionError: '2016-12-16T18:38:14.000Z' == '2016-12-16T13:38:14-0500'

The only issue is date formatting:
AssertionError: '2016-12-16T18:38:14.000Z' == '2016-12-16T13:38:14-0500'
@kev5873
Copy link
Contributor

kev5873 commented Feb 2, 2017

Hmm... I'm not sure why that test failed, since the two dates are equivalent. I've updated the test to use 2016-12-16T18:38:14.000Z since this seems to be the standard format across the other tests.

var a = new Date('2016-12-16T18:38:14.000Z');
var b = new Date('2016-12-16T13:38:14-0500');
a.getTime() == b.getTime(); // This is true.

@kev5873 kev5873 requested a review from adampash February 2, 2017 17:17
Copy link
Contributor

@adampash adampash left a comment

Choose a reason for hiding this comment

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

Yep, that's right. The test is testing the literal string output, because that's what the API returns — an ISO-8601 formatted date (i.e., date.toISOString()). So it's not a question of whether the two dates are equivalent — they definitely are — it's a test of the string output from the API.

Anyway... LGTM! 👍

@kev5873
Copy link
Contributor

kev5873 commented Feb 2, 2017

Cool, is that something that should be fixed for a more literal date comparison?

@kev5873 kev5873 merged commit d292d8e into master Feb 2, 2017
@kev5873 kev5873 deleted the feat-nydailynews-extractor branch February 2, 2017 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants