feat: adds Date support to escapeCQL function #131

Merged
merged 6 commits into from Dec 15, 2013

Conversation

Projects
None yet
2 participants
@tim-dev
Contributor

tim-dev commented Nov 14, 2013

Just allows for handling of the Date object.

@devdazed

This comment has been minimized.

Show comment
Hide comment
@devdazed

devdazed Nov 14, 2013

Member

hey thanks! lets write some tests for it, then I think it will be good to merge.

Member

devdazed commented Nov 14, 2013

hey thanks! lets write some tests for it, then I think it will be good to merge.

@tim-dev

This comment has been minimized.

Show comment
Hide comment
@tim-dev

tim-dev Nov 14, 2013

Contributor

Cool, will do. I'm thinking pre-1970 date, current date, and future date. Should we worry about invalid dates or leave that to the developer?

Contributor

tim-dev commented Nov 14, 2013

Cool, will do. I'm thinking pre-1970 date, current date, and future date. Should we worry about invalid dates or leave that to the developer?

@devdazed

This comment has been minimized.

Show comment
Hide comment
@devdazed

devdazed Nov 14, 2013

Member

i think we should check for an invalid date, C* may not like the response given by one

> d = new Date('asdfsg')
Invalid Date
> d instanceof Date
true
> d.getTime()
NaN
Member

devdazed commented Nov 14, 2013

i think we should check for an invalid date, C* may not like the response given by one

> d = new Date('asdfsg')
Invalid Date
> d instanceof Date
true
> d.getTime()
NaN
@tim-dev

This comment has been minimized.

Show comment
Hide comment
@tim-dev

tim-dev Nov 15, 2013

Contributor

I can verify that C* doesn't want a NaN. I'll have it throw if getTime returns a NaN.

Contributor

tim-dev commented Nov 15, 2013

I can verify that C* doesn't want a NaN. I'll have it throw if getTime returns a NaN.

@tim-dev

This comment has been minimized.

Show comment
Hide comment
@tim-dev

tim-dev Nov 15, 2013

Contributor

Ok, I think that'll do it. Had to use an ISO date string rather than an integer because C* < 2 complained about negative integers being invalid.

Contributor

tim-dev commented Nov 15, 2013

Ok, I think that'll do it. Had to use an ISO date string rather than an integer because C* < 2 complained about negative integers being invalid.

@devdazed

This comment has been minimized.

Show comment
Hide comment
@devdazed

devdazed Nov 16, 2013

Member

Interesting, is there a reason why toISOString() didn't work? also, from the looks of it, this will limit the resolution to the second rather than the millisecond?

Member

devdazed commented Nov 16, 2013

Interesting, is there a reason why toISOString() didn't work? also, from the looks of it, this will limit the resolution to the second rather than the millisecond?

@tim-dev

This comment has been minimized.

Show comment
Hide comment
@tim-dev

tim-dev Nov 16, 2013

Contributor

After some more in-depth research, here are the major hurdles:

My third commit had different logic for negative pre-epoch times, and tried to use toISOString(), but that failed with an unable to coerce to a formatted date error in all C* versions due to the patterns mentioned above.

As the date string loses millisecond precision, I'm thinking it should use getTime() for all but the pre-epoch dates, which I assume no longer care about millisecond precision. To get fully smart, it could examine the C* version and only use the date format for earlier versions. Do you happen to know off the top of your head how to check the C* version here, or if that's even reasonable?

Contributor

tim-dev commented Nov 16, 2013

After some more in-depth research, here are the major hurdles:

My third commit had different logic for negative pre-epoch times, and tried to use toISOString(), but that failed with an unable to coerce to a formatted date error in all C* versions due to the patterns mentioned above.

As the date string loses millisecond precision, I'm thinking it should use getTime() for all but the pre-epoch dates, which I assume no longer care about millisecond precision. To get fully smart, it could examine the C* version and only use the date format for earlier versions. Do you happen to know off the top of your head how to check the C* version here, or if that's even reasonable?

@tim-dev

This comment has been minimized.

Show comment
Hide comment
@tim-dev

tim-dev Nov 18, 2013

Contributor

I commited a change that uses the long unless it's negative. Let me know if that passes muster.

Contributor

tim-dev commented Nov 18, 2013

I commited a change that uses the long unless it's negative. Let me know if that passes muster.

devdazed added a commit that referenced this pull request Dec 15, 2013

Merge pull request #131 from tim-dev/master
feat: adds Date support to escapeCQL function

@devdazed devdazed merged commit 5905031 into simplereach:master Dec 15, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment