util,test: Use consistent Date representation for util/inspect by Xotic750 · Pull Request #4318 · nodejs/node

Xotic750 · 2015-12-17T00:16:24Z

Re: #4314
Inspect formats dates in two different ways, depending on
whether it has properties or not.

No properties and it uses toString.
https://github.com/nodejs/node/blob/master/lib/util.js#L272

The toString() method always returns a string representation of
the date in American English.

While when properties are present it uses toUTCString.
https://github.com/nodejs/node/blob/master/lib/util.js#L393

The format of the return value may vary according to the platform.
The most common return value is a RFC-1123 formatted date stamp,
which is a slightly updated version of RFC-822 date stamps.

I am wondering why two different representations are used?

And being 2015, it would seem sensible to use toISOString.

The toISOString() method returns a string in simplified
extended ISO format (ISO 8601), which is always 24 characters
long: YYYY-MM-DDTHH:mm:ss.sssZ. The timezone is always zero
UTC offset, as denoted by the suffix "Z".

./configure
make test
Total errors found: 0

jasnell · 2015-12-17T01:01:16Z

Thanks @Xotic750 ! Just a heads up tho, we have style guidelines for commit logs documented here. If it all possible, it would be helpful if you could update the log but it's possible to fix if/when the PR gets landed also.

jasnell · 2015-12-17T01:01:52Z

LGTM but I'd like to get review from others @nodejs/collaborators

indutny · 2015-12-17T01:15:02Z

LGTM

Fix: #4314 PR-URL: #4318 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

Re: nodejs#4314 `Inspect` formats dates in two different ways, depending on whether it has properties or not. No properties and it uses `toString`. https://github.com/nodejs/node/blob/master/lib/util.js#L272 > The toString() method always returns a string representation of > the date in American English. While when properties are present it uses `toUTCString`. https://github.com/nodejs/node/blob/master/lib/util.js#L393 > The format of the return value may vary according to the platform. > The most common return value is a RFC-1123 formatted date stamp, > which is a slightly updated version of RFC-822 date stamps. I am wondering why two different representations are used? And being 2015, it would seem sensible to use `toISOString`. > The toISOString() method returns a string in simplified > extended ISO format (ISO 8601), which is always 24 characters > long: YYYY-MM-DDTHH:mm:ss.sssZ. The timezone is always zero > UTC offset, as denoted by the suffix "Z". ./configure make test Total errors found: 0

evanlucas · 2015-12-17T01:16:28Z

LGTM. This should be semver-major?

indutny · 2015-12-17T01:16:29Z

Landed in 04af258, thank you!

indutny · 2015-12-17T01:16:46Z

@evanlucas it should be, agree

jasnell · 2015-12-17T01:19:41Z

@indutny ... thanks for landing... I think for semver-major type things tho we may need to give it a bit more time for review before landing

Xotic750 · 2015-12-17T01:22:03Z

I didn't realise that there was a specific format for PRs, sorry about that.

ofrobots · 2015-12-17T01:25:55Z

It seems like this got landed before the commit message was fixed in fa0442c.

indutny · 2015-12-17T01:29:10Z

@jasnell gosh when reading "others" I actually read "other". So I thought that you needed just one more LGTM. Sorry!

@ofrobots I have fixed the log before landing it 04af258

jasnell · 2015-12-17T01:37:36Z

@indutny ... unfortunately the log in the landed commit is still incorrect: 04af258 (the log subject is the wrong format)

Fix: #4314 PR-URL: #4318 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

cjihrig · 2015-12-17T03:18:16Z

I have fixed the commit message. Some time had passed, but it was still the most recent commit. See 93d6b5f.

indutny · 2015-12-17T03:51:46Z

Thank you @cjihrig ! I was away.

Sorry everyone for confusing and landing it too fast with mistakes :(

rvagg · 2016-01-04T03:28:31Z

Hey @Xotic750, I believe this is your first commit to core! Thanks for picking up this inconsistency, the change will make it in to v6 in a few months time. Welcome on board!

Xotic750 · 2016-01-04T12:05:19Z

@rvagg Yes, this was my first commit. I've been using node for some time now, so I guess it's about time I did something useful. Thanks. ;)

Fix: nodejs#4314 PR-URL: nodejs#4318 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

mscdex added the util Issues and PRs related to the built-in util module. label Dec 17, 2015

indutny pushed a commit that referenced this pull request Dec 17, 2015

Use consistent Date representation for util/inspect

04af258

Fix: #4314 PR-URL: #4318 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

indutny added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 17, 2015

Xotic750 force-pushed the util-inspect-date branch from 6b58ad3 to fa0442c Compare December 17, 2015 01:18

jasnell closed this Dec 17, 2015

Xotic750 changed the title ~~Use consistent Date representation for util/inspect~~ util,test: Use consistent Date representation for util/inspect Dec 17, 2015

cjihrig pushed a commit that referenced this pull request Dec 17, 2015

util: use consistent Dates in inspect()

93d6b5f

Fix: #4314 PR-URL: #4318 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

util: use consistent Dates in inspect()

4c1dd4a

Fix: nodejs#4314 PR-URL: nodejs#4318 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

addaleax mentioned this pull request Dec 12, 2016

Different timestamp created by node.js and browser #9805

Closed

jmendiara mentioned this pull request May 1, 2017

NodeJS 6 backwards compatible support telefonicaid/logops#36

Closed

Uh oh!

Conversation

Xotic750 commented Dec 17, 2015

Uh oh!

jasnell commented Dec 17, 2015

Uh oh!

jasnell commented Dec 17, 2015

Uh oh!

indutny commented Dec 17, 2015

Uh oh!

evanlucas commented Dec 17, 2015

Uh oh!

indutny commented Dec 17, 2015

Uh oh!

indutny commented Dec 17, 2015

Uh oh!

jasnell commented Dec 17, 2015

Uh oh!

Xotic750 commented Dec 17, 2015

Uh oh!

ofrobots commented Dec 17, 2015

Uh oh!

indutny commented Dec 17, 2015

Uh oh!

jasnell commented Dec 17, 2015

Uh oh!

cjihrig commented Dec 17, 2015

Uh oh!

indutny commented Dec 17, 2015

Uh oh!

rvagg commented Jan 4, 2016

Uh oh!

Xotic750 commented Jan 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants