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

Use Date#toISOString() instead to Date#toUTCString() when output is n… #418

Closed
wants to merge 1 commit into from

Conversation

denouche
Copy link
Contributor

@denouche denouche commented Jan 29, 2017

Use Date#toISOString() instead to Date#toUTCString() when output is not a TTY, which:

  • is easier to parse (does not contains spaces, day name, and everything is written with numbers)
  • contains milliseconds

…ot a TTY, which is easier to parse and contains milliseconds
@@ -76,7 +76,7 @@ Then, run the program to be debugged as usual.

![](http://f.cl.ly/items/2i3h1d3t121M2Z1A3Q0N/Screenshot.png)

When stdout is not a TTY, `Date#toUTCString()` is used, making it more useful for logging the debug information as shown below:
When stdout is not a TTY, `Date#toISOString()` is used, making it more useful for logging the debug information as shown below:

![](http://f.cl.ly/items/112H3i0e0o0P0a2Q2r11/Screenshot.png)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

by the way this screenshot should also be updated, but I don't have to right term to do that.

@coveralls
Copy link

coveralls commented Jan 29, 2017

Coverage Status

Coverage remained the same at 63.75% when pulling df89099 on denouche:toisostring into 37e14d6 on visionmedia:master.

@thebigredgeek
Copy link
Contributor

This will probably need to wait until v3... and should be handled outside of the normal lib through the application of middleware

@medikoo
Copy link
Contributor

medikoo commented Jul 17, 2017

@thebigredgeek what is the state of v3? Is launch date near? Wouldn't it be better to accept such small improvements to v2 when v3 doesn't seem anywhere close?

Using Date#toISOString instead to Date#toUTCString indeed would be way way better +1

@thebigredgeek
Copy link
Contributor

I am just now getting back into the saddle with OSS stuff. My last job took a bit of a toll, and I just switched to a new role at a new company. I will get together with a couple of other core contributors and put together an estimated release date

@medikoo
Copy link
Contributor

medikoo commented Jul 20, 2017

@thebigredgeek I fully understand, it's very hard to meet promised delivery dates when non reimbursed OSS work is concerned, I failed on that myself numerous times.

@medikoo
Copy link
Contributor

medikoo commented Jul 21, 2017

@thebigredgeek anyway, why not bring this update in a meantime to v2? It's nearly one liner and improves things until we have v3, of which delivery date is not very close as I assume.

@rauchg
Copy link

rauchg commented Jul 27, 2017

This is pretty critical IMO. I would at least re-open this PR.

@thebigredgeek
Copy link
Contributor

I don't mind reopening, but this is a breaking change. I am not sure it would be a good idea to release this on a minor or patch semver. The API is in flux (we need to find time to finish up v3) so I am hesitant to solve this (personally) until we are ready to release another major semver. I am down for discussion, though. Just haven't had much free time of late

@thebigredgeek thebigredgeek reopened this Aug 4, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 63.804% when pulling df89099 on denouche:toisostring into 37e14d6 on visionmedia:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 63.804% when pulling df89099 on denouche:toisostring into 37e14d6 on visionmedia:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 63.804% when pulling df89099 on denouche:toisostring into 37e14d6 on visionmedia:master.

@medikoo
Copy link
Contributor

medikoo commented Aug 5, 2017

I don't mind reopening, but this is a breaking change. I am not sure it would be a good idea to release this on a minor or patch semver.

@thebigredgeek It's breaking if we assume that currently outputted format is parsable, so may break things for some ends which tries to parse it programmatically. However what's interesting this improvement is about making this output parsable :) So it can also be understood as fix to that problem.

Anyway I can understand the worry, as there can be some corner-case scenario where this will break things. Still in such case I would just publish it as v3.0. When following semver we shouldn't be emotionally attached to version numbers (it should not be combined with marketing). So changes that are planned now for v3, will eventually land at v4 or maybe even v5 ;-)

@TooTallNate
Copy link
Contributor

I'm 👍 for this change, and also tend to agree with @medikoo about not being emotionally attached to a particular version number. Maybe we land this and prepare a v3, and expect some of the bigger planned changes for v4 or whatever.

@TooTallNate TooTallNate mentioned this pull request Aug 8, 2017
TooTallNate pushed a commit that referenced this pull request Aug 8, 2017
…ot a TTY

Easier to parse programatically and contains milliseconds.

Closes #418.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants