Skip to content

Support for temporal types #341

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

Merged
merged 10 commits into from
Apr 3, 2018
Merged

Support for temporal types #341

merged 10 commits into from
Apr 3, 2018

Conversation

lutovich
Copy link
Contributor

@lutovich lutovich commented Mar 27, 2018

This PR makes driver able to receive temporal types as query results and send them as query parameters. All types are represented by custom date-time classes because native Date type is quite limited. It can't represent needed millisecond range and does not have support for things like timezone.

New types are:

  • Duration - represents Duration Cypher type
  • LocalTime - represents LocalTime Cypher type
  • Time - represents Time Cypher type
  • Date - represents Date Cypher type
  • LocalDateTime - represents LocalDateTime Cypher type
  • DateTimeWithZoneOffset - represents DateTime Cypher type with time zone as offset in seconds
  • DateTimeWithZoneOffset - represents DateTime Cypher type with time zone as ID

All temporal types expose understandable properties. For example Date has year, month and day properties. Driver uses code adapted from https://github.com/ThreeTen/threetenbp to perform the conversions. Main difference is that driver operates on Integer objects and not native JavaScript numbers.

Temporal types also override toString() to return ISO 8601 strings.

@lutovich lutovich requested a review from ali-ince March 27, 2018 22:26
This commit makes driver able to receive temporal types as query results and
send them as query parameters. All types are represented by custom date-time
classes because native `Date` type is quite limited. It can't represent
needed millisecond range and does not have support for things like timezone.

All temporal types expose understandable properties. For example `CypherDate`
has `year`, `month` and `day` properties. Driver uses code adapted from
https://github.com/ThreeTen/threetenbp to perform the conversions. Main
difference is that driver operates on `Integer` objects and not native
JavaScript numbers.
It returns ISO strings for every temporal type except DateTime with
time zone ID. Later is formatted to an ISO date-time and time zone ID is
appended in square brackets. Ideally driver would also include offset
but there is no way to get it without a third-party library. Offset
might be included in future.
And use top-level import in temporal tests.
TCK rewrites ids of nodes and relationships in received paths with fake
ids that start from zero. This is done to easily compare predefined
path from feature file with received path. It makes entity ids
predictable.

This commit removes incorrect assertion during path rewriting.
Previously this assertion did not fail because `Integer#notEquals()`
always returned a falsy value due to a bug.
Copy link
Contributor

@ali-ince ali-ince left a comment

Choose a reason for hiding this comment

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

LGTM.

Only one very small comment on variable naming.

}

function packLocalTime(value, packer, onError) {
const totalNanos = cypherLocalTimeToNanoOfDay(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe name this as nanoOfDay instead of totalNanos?

* @param {function} onError the error callback.
*/
function packTime(value, packer, onError) {
const totalNanos = cypherLocalTimeToNanoOfDay(value.localTime);
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?

They will mostly be used with `neo4j.` namespace so "Cypher" prefix
seems unnecessary. Types can also be imported with an alias.
@lutovich lutovich merged commit a63405b into neo4j:1.6 Apr 3, 2018
@lutovich lutovich deleted the 1.6-temporal branch April 3, 2018 15:33
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