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: accept entry.timestamp string input in RFC3339 format #937

Merged
merged 5 commits into from
Nov 9, 2020
Merged

Conversation

0xSage
Copy link
Contributor

@0xSage 0xSage commented Nov 6, 2020

Fixes #934

Context:

  • Keeps feature parity with API
  • Users may find this useful when they need timestamp to be be higher precision than milliseconds, and want to control timestamp input themselves.

@0xSage 0xSage added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Nov 6, 2020
@0xSage 0xSage requested review from a team as code owners November 6, 2020 23:59
@0xSage 0xSage self-assigned this Nov 6, 2020
@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/nodejs-logging API. label Nov 6, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2020
@codecov
Copy link

codecov bot commented Nov 7, 2020

Codecov Report

Merging #937 (1f1214b) into master (4325e3b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #937   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files          18       18           
  Lines       10928    10940   +12     
  Branches      367      372    +5     
=======================================
+ Hits        10733    10745   +12     
  Misses        193      193           
  Partials        2        2           
Impacted Files Coverage Δ
src/entry.ts 100.00% <100.00%> (ø)

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 4325e3b...1f1214b. Read the comment docs.

Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

Nit on usage of parseInt, but otherwise LGTM

src/entry.ts Outdated
const nanoSecs = zuluTime.match(reNano)?.[1];
entry.timestamp = {
seconds: ms ? Math.floor(ms / 1000) : 0,
nanos: nanoSecs ? parseInt(nanoSecs.padEnd(9, '0')) : 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's generally safer to use the Number constructor instead of parseInt. parseInt is more liberal in what it accepts, and in some cases can make assumptions about the radix (leading 0s and octal numbers, etc).

@0xSage 0xSage merged commit 869bbaf into master Nov 9, 2020
@0xSage 0xSage deleted the fix934 branch November 9, 2020 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/nodejs-logging API. cla: yes This human has signed the Contributor License Agreement. priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entry: should let users input custom timestamp string
2 participants