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(timestamp): create new date/timestamp classes #517

Merged
merged 9 commits into from
Feb 25, 2019
Merged

feat(timestamp): create new date/timestamp classes #517

merged 9 commits into from
Feb 25, 2019

Conversation

callmehiphop
Copy link
Contributor

@callmehiphop callmehiphop commented Feb 8, 2019

Fixes #494
Fixes #526
Closes #529

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Timestamps

This PR brings a new class for dealing with Cloud Spanner Timestamps. Previously we would return Date objects, but these were only good for millisecond precision. Since timestamps can come from the API in the form of either an ISO string or a protobuf timestamp, the class can handle either allowing users to easily use commit and read timestamps in queries, as well as result timestamps in snapshot bounds.

With ISO string.

const timestamp = Spanner.timestamp('2019-02-08T10:34:29.481145231Z');

With protobuf timestamp.

const [seconds, nanos] = process.hrtime();
const timestamp = Spanner.timestamp({seconds, nanos});

The Timestamp class extends the native Date class, so users can perform basic operations to the Timestamp if they choose.

timestamp.setNanoseconds(10);

console.log(timestamp.toISOString());
// => "2019-02-08T10:34:29.481145010Z"

It will also accept any single value that the Date object would accept in scenarios where micro-precision isn't important.

const timestamp = Spanner.timestamp(Date.now());

console.log(timestamp.toISOString());
// => "1985-06-03T22:37:14.312Z"

SpannerDate

To keep dealing with date times consistent, I've also refactored the SpannerDate class to extend the native Date class. It accepts any single value that the Date class would also accept.

const date = Spanner.date('6-3-1985');

console.log(date.toISOString());
// => "1985-06-03T00:00:00.000Z"

console.log(date.toJSON());
// => "1985-06-03"

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2019
@codecov
Copy link

codecov bot commented Feb 10, 2019

Codecov Report

Merging #517 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
+ Coverage   89.87%   89.95%   +0.07%     
==========================================
  Files          15       15              
  Lines        1432     1443      +11     
  Branches       42       42              
==========================================
+ Hits         1287     1298      +11     
  Misses        140      140              
  Partials        5        5
Impacted Files Coverage Δ
src/index.ts 99.1% <100%> (+0.02%) ⬆️
src/codec.ts 97.65% <100%> (+0.17%) ⬆️
src/transaction.ts 98.82% <100%> (-0.01%) ⬇️
src/batch-transaction.ts 97.29% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d712018...ae1dc67. Read the comment docs.

@callmehiphop
Copy link
Contributor Author

@JustinBeckwith I'm not sure what your feelings are on my Timestamp class, but assuming it is something we would like to move forward with, I think it would be better suited for @google-cloud/common since we might be able to utilize it elsewhere (googleapis/nodejs-bigquery#6)

@JustinBeckwith
Copy link
Contributor

I'm +1 to exposing nanosecond precision in a reusable way across our libs. Is common really the right place for it though? Thinking out loud:

  1. I have a general aversion to the notion of a common module, as it usually turns into a dumping ground. Though I get for many of our modules it makes sense.
  2. It sounds like this could be of general use to the rest of the node community. Maybe it makes sense to have our own individual npm module just for nanosecond precision dates?

Adding @ofrobots and @googleapis/node-team as they will know if there's already something in the ecosystem we should be using, or if what you got here is of enough value to the community to spin out :)

@JustinBeckwith
Copy link
Contributor

Oh also @callmehiphop silly question - is this a breaking change?

@callmehiphop
Copy link
Contributor Author

Yeah, technically it is, I hadn't released our latest major breaking change yet so my plan was to ship this with it.

src/codec.ts Outdated Show resolved Hide resolved
src/codec.ts Outdated Show resolved Hide resolved
@ofrobots
Copy link

How does this compare to something like nano-date?

@JustinBeckwith
Copy link
Contributor

😨
image

@ofrobots
Copy link

ofrobots commented Feb 11, 2019

My preference would be a new public module for this. This seems like a feature anybody could take advantage of. IMHO there is no reason to sequester it into the common module. Two ways I see this can happen are:

  1. See if we can contribute to the existing nano-date module.
  2. Create a new module (date-nano?)

@callmehiphop callmehiphop added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 14, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 15, 2019
@callmehiphop callmehiphop added status: blocked Resolving the issue is dependent on other work. and removed 🚨 This issue needs some love. labels Feb 19, 2019
@callmehiphop
Copy link
Contributor Author

@stephenplusplus I made some changes to the SpannerDate class per #526 if you wouldn't mind giving it another go through D:

@callmehiphop callmehiphop removed the status: blocked Resolving the issue is dependent on other work. label Feb 25, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 25, 2019
@callmehiphop callmehiphop removed the 🚨 This issue needs some love. label Feb 25, 2019
@callmehiphop callmehiphop removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 25, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 25, 2019
@JustinBeckwith JustinBeckwith merged commit c8130ef into googleapis:master Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants