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

util,test: Use consistent Date representation for util/inspect #4318

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@Xotic750
Copy link

commented Dec 17, 2015

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

@mscdex mscdex added the util label Dec 17, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

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

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

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

@indutny

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

LGTM

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

Use consistent Date representation for util/inspect
Fix: #4314
PR-URL: #4318
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
util,test: Use consistent Date representation for util/inspect
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
@evanlucas

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

LGTM. This should be semver-major?

@indutny

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

Landed in 04af258, thank you!

@indutny

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

@evanlucas it should be, agree

@Xotic750 Xotic750 force-pushed the Xotic750:util-inspect-date branch to fa0442c Dec 17, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

@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

@jasnell jasnell closed this Dec 17, 2015

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

@Xotic750

This comment has been minimized.

Copy link
Author

commented Dec 17, 2015

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

@ofrobots

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2015

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

@indutny

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

@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

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

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

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

util: use consistent Dates in inspect()
Fix: #4314
PR-URL: #4318
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2015

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

@indutny

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

Thank you @cjihrig ! I was away.

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

@rvagg

This comment has been minimized.

Copy link
Member

commented Jan 4, 2016

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

This comment has been minimized.

Copy link
Author

commented Jan 4, 2016

@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. ;)

@jasnell jasnell referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

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

util: use consistent Dates in inspect()
Fix: nodejs#4314
PR-URL: nodejs#4318
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.