Skip to content

Flatten all temporal types #345

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 9, 2018
Merged

Flatten all temporal types #345

merged 3 commits into from
Apr 9, 2018

Conversation

lutovich
Copy link
Contributor

@lutovich lutovich commented Apr 5, 2018

And unit tests for temporal-util.

@lutovich lutovich requested a review from ali-ince April 5, 2018 13:01
lutovich added 2 commits April 5, 2018 17:27
@lutovich
Added unit tests for temporal-util
a600c83
@lutovich
Turn all temporal types into flat objects
456ae14 
Previously some temporal types were modeled as a combination of other
temporal types. For example `Time` contained an instance of `LocalTime`
and an offset in seconds. This made constructors more complicated and
getters needed to access nested properties. For example `Time.hour`
would need to be accessed as `Time.localTime.hour`.

This commit makes `Time`, `LocalDateTime`, `DateTimeWithZoneOffset`
and `DateTimeWithZoneId` contain primitive properties and not use
other temporal types as building blocks. Also made all properties of
spatial and temporal types readonly in TypeScript declarations.
@lutovich lutovich force-pushed the 1.6-temporal-getters branch from 8b21872 to 456ae14 Compare April 5, 2018 15:54
@lutovich lutovich changed the title Getters for temporal type components Flatten all temporal types Apr 5, 2018
ali-ince
ali-ince approved these changes Apr 9, 2018
View reviewed changes
Copy link
Contributor

@ali-ince ali-ince left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

I've only one question. What's the motivation behind adding string as a possible input type to the functions in temporal-util.js? Does it provide the user to pass in a map like this as a temporal value?

{
  year: "1990",
  month: "12",
  day: "5"
}

@lutovich
Copy link
Contributor Author

lutovich commented Apr 9, 2018

@ali-ince thanks for the review!
Internally all arguments are converted to Integer using int() function. It supports strings as parameters. That is why JSDocs mention strings. temporal-util is not public and public docs say nothing about strings.

@lutovich lutovich merged commit e3006a5 into neo4j:1.6 Apr 9, 2018
@lutovich lutovich deleted the 1.6-temporal-getters branch April 9, 2018 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ali-ince ali-ince ali-ince approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lutovich @ali-ince