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

Allow to create temporal objects from standard JS Date #390

Merged
merged 3 commits into from
Jun 28, 2018

Conversation

lutovich
Copy link
Contributor

This PR adds #fromStandardDate() functions to all temporal types except Duration. Such functions allow to create temporal objects from the provided standard JavaScript Date.

@lutovich lutovich requested a review from ali-ince June 19, 2018 16:45
This commit adds `#fromStandardDate()` functions to all temporal types
except `Duration`. Such functions allow to create temporal objects
from the provided standard JavaScript `Date`.
@lutovich lutovich force-pushed the 1.6-from-std-date branch from 6e679bc to c6c1a08 Compare June 20, 2018 09:31
@ali-ince ali-ince self-assigned this Jun 21, 2018

return new Date(
standardDate.getFullYear(),
standardDate.getMonth(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a problem about conversion of month value from/to standard date as javascript Date object stores months as 0 based values - with 0 = January and 11 = December. It seems to me that we do not consider this difference in conversions.

Maybe add value range checks on temporal type constructors?

}

/**
* Get the total number of nanoseconds from the given standard JavaScript date.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the comment is outdated.


return new LocalDateTime(
standardDate.getFullYear(),
standardDate.getMonth(),
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.


return new DateTime(
standardDate.getFullYear(),
standardDate.getMonth(),
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.

@@ -658,6 +659,147 @@ describe('temporal-types', () => {
expect(() => new neo4j.types.DateTime(1, 2, 3, 4, 5, 6, 7, 8, 'UK')).toThrow();
});

it('should convert standard Date to neo4j LocalTime', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some integration tests that incorporate standard date -> temporal type conversions?

lutovich added 2 commits June 21, 2018 12:28
Standard dates have zero-based month. Neo4j temporal types have
1-based month. Conversion from standard date with zero month was not
handled correctly and resulted in zero month in neo4j temporal types.
@lutovich lutovich requested a review from oskarhane June 21, 2018 15:02
@lutovich lutovich changed the base branch from 1.7 to 1.6 June 25, 2018 15:45
Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

Good stuff, I have nothing to add!

@lutovich lutovich merged commit dd1187d into neo4j:1.6 Jun 28, 2018
@lutovich lutovich deleted the 1.6-from-std-date branch June 28, 2018 14:50
@ivan-kleshnin
Copy link

Any reason to not apply this to native JS dates automatically?

@lutovich
Copy link
Contributor Author

@ivan-kleshnin the problem is that driver can't know which Neo4j type should the native JS date be converted to. It can be converted to either LocalDateTime or DateTime but driver can't know if time zone information should be preserved or not. Technically, native JS dates can also get converted to Date, LocalTime or Time by using a subset of available getters. Only use getFullYear(), getMonth() and getDate() to construct Neo4j Date from a native date, ignore getHours(), etc.

That's why this PR adds explicit conversion methods so application code can tell driver which Neo4j temporal type is required.

Does this make sense to you?

@ivan-kleshnin
Copy link

ivan-kleshnin commented Aug 20, 2018

@lutovich yes, thanks. That what I thought, but as a Neo4j noob wanted to ensure :)

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.

4 participants