Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

encode date is null fix #85

Merged
merged 7 commits into from
Dec 16, 2020
Merged

encode date is null fix #85

merged 7 commits into from
Dec 16, 2020

Conversation

zheli-1
Copy link
Contributor

@zheli-1 zheli-1 commented Nov 18, 2020

No description provided.

@zheli-1
Copy link
Contributor Author

zheli-1 commented Nov 18, 2020

Referring to the issue I submitted :D
Hi, there.

I am running an auto script testing framework and found out it might be a bug in encoder.js:

it's when encoderDate's argument is null, it throws exceptions so could a condition to avoid this.

Can you verify if it's a potential bug? it would be helpful for my experiment:)

function encodeDate (dt) {
var encoded
//filter
if(dt === null){
return;
}
//zl3
var millis = dt * 1
var seconds = Math.floor(millis / 1000)
var nanos = (millis - (seconds * 1000)) * 1E6

if (nanos || seconds > 0xFFFFFFFF) {
After adding the following line, in test scripting timestamps.js passes.
I will send a pull request if necessary :)

@mcollina
Copy link
Owner

there seems to be linting errors: https://travis-ci.org/github/mcollina/msgpack5/jobs/744518226

@zheli-1
Copy link
Contributor Author

zheli-1 commented Dec 9, 2020

It seems to be a problem with my format. I fixed it.
sorry for the delayed reply.

@mcollina
Copy link
Owner

Can you please add a unit test?

@zheli-1
Copy link
Contributor Author

zheli-1 commented Dec 15, 2020

Hi, I have added a unit test for that.

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good work, a could of nits.

if (dt === null) {
return
}
// zhe-li fix
Copy link
Owner

Choose a reason for hiding this comment

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

why these comments? can you remove them?

@@ -0,0 +1,12 @@
var test = require('tape').test
Copy link
Owner

Choose a reason for hiding this comment

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

'use strict' is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fix all that:)

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

This was referenced Mar 12, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants