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.

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

Using the new Timestamp class in the public API #226

Merged
merged 14 commits into from Jun 28, 2018
Merged

Using the new Timestamp class in the public API #226

merged 14 commits into from Jun 28, 2018

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Jun 26, 2018 •
edited

This is a breaking API change that uses the new Timestamp type instead of ISO Strings in all places where we need nanosecond precision.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 26, 2018
@codecov
Copy link

codecov bot commented Jun 26, 2018 •
edited

Codecov Report

Merging #226 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #226   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines        1834   1837    +3     
=====================================
+ Hits         1834   1837    +3
Impacted Files Coverage Δ
src/transaction.js 100% <ø> (ø) ⬆️
src/timestamp.js 100% <100%> (ø) ⬆️
src/watch.js 100% <100%> (ø) ⬆️
src/index.js 100% <100%> (ø) ⬆️
src/write-batch.js 100% <100%> (ø) ⬆️
src/document.js 100% <100%> (ø) ⬆️
src/reference.js 100% <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 031da1c...ddbcacc. Read the comment docs.

var-const
var-const suggested changes Jun 26, 2018
View reviewed changes
Copy link
Contributor

@var-const var-const left a comment

Choose a reason for hiding this comment

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

Only nitpicks. It's been a while since I touched JSDoc, so my comments might be inapplicable.

src/document.js
* @param {Timestamp} readTime - The time when this snapshot was read.
* @param {Timestamp=} createTime - The time when the document was created
* (or undefined if the document does not exist).
* @param {Timestamp=} updateTime - The time when the document was last
* updated (or undefined if the document does not exist).

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/document.js
* @param {string=} updateTime - The ISO 8601 time when the document was last
* @param {Timestamp} readTime - The time when this snapshot was read.
* @param {Timestamp=} createTime - The time when the document was created
* (or undefined if the document does not exist).

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/reference.js Outdated
* document was last updated at lastUpdateTime (as ISO 8601 string). Fails the
* delete if the document was last updated at a different time.
* @param {Timestamp=} precondition.lastUpdateTime If set, enforces that the
* document was last updated at lastUpdateTime. Fails the elete if the

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/reference.js Outdated
* delete if the document was last updated at a different time.
* @param {Timestamp=} precondition.lastUpdateTime If set, enforces that the
* document was last updated at lastUpdateTime. Fails the elete if the
* ddocument was last updated at a different time.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/timestamp.js
*/
static fromProto(timestamp) {
return new Timestamp(
Number(timestamp.seconds || 0),

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/write-batch.js
@@ -40,11 +40,16 @@ let DocumentMask;
*/
let DocumentTransform;

/*
/*!

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/document.js
* @param {string=} createTime - The ISO 8601 time when the document was
* created (or undefined if the document does not exist).
* @param {string=} updateTime - The ISO 8601 time when the document was last
* @param {Timestamp} readTime - The time when this snapshot was read.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

schmidt-sebastian
schmidt-sebastian commented Jun 26, 2018
View reviewed changes
Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Thanks for your review. Sorry for pushing back on most of your feedback with the lamest response in software engineering (that I want to match the rest of the docs).

src/document.js
* @param {string=} createTime - The ISO 8601 time when the document was
* created (or undefined if the document does not exist).
* @param {string=} updateTime - The ISO 8601 time when the document was last
* @param {Timestamp} readTime - The time when this snapshot was read.

This comment was marked as spam.

Sign in to view

src/document.js
* @param {string=} updateTime - The ISO 8601 time when the document was last
* @param {Timestamp} readTime - The time when this snapshot was read.
* @param {Timestamp=} createTime - The time when the document was created
* (or undefined if the document does not exist).

This comment was marked as spam.

Sign in to view

src/document.js
* @param {Timestamp} readTime - The time when this snapshot was read.
* @param {Timestamp=} createTime - The time when the document was created
* (or undefined if the document does not exist).
* @param {Timestamp=} updateTime - The time when the document was last
* updated (or undefined if the document does not exist).

This comment was marked as spam.

Sign in to view

src/reference.js Outdated
* delete if the document was last updated at a different time.
* @param {Timestamp=} precondition.lastUpdateTime If set, enforces that the
* document was last updated at lastUpdateTime. Fails the elete if the
* ddocument was last updated at a different time.

This comment was marked as spam.

Sign in to view

src/reference.js Outdated
* document was last updated at lastUpdateTime (as ISO 8601 string). Fails the
* delete if the document was last updated at a different time.
* @param {Timestamp=} precondition.lastUpdateTime If set, enforces that the
* document was last updated at lastUpdateTime. Fails the elete if the

This comment was marked as spam.

Sign in to view

src/timestamp.js
*/
static fromProto(timestamp) {
return new Timestamp(
Number(timestamp.seconds || 0),

This comment was marked as spam.

Sign in to view

src/write-batch.js
@@ -40,11 +40,16 @@ let DocumentMask;
*/
let DocumentTransform;

/*
/*!

This comment was marked as spam.

Sign in to view

@schmidt-sebastian schmidt-sebastian changed the base branch from timestamps to master June 26, 2018 22:39
@schmidt-sebastian schmidt-sebastian merged commit 6c1f409 into master Jun 28, 2018
@schmidt-sebastian schmidt-sebastian deleted the usetimestamps branch July 24, 2018 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenplusplus stephenplusplus stephenplusplus approved these changes

@var-const var-const var-const approved these changes

Assignees

@stephenplusplus stephenplusplus

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@schmidt-sebastian @stephenplusplus @var-const @googlebot