FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

console: coerce label to string in console.time() by jasnell · Pull Request #14643 · nodejs/node · GitHub

/ node Public

console: coerce label to string in console.time() - #14643

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:console-standard-fixes
ConversationCommits (2)ChecksFiles changed
Closed

console: coerce label to string in console.time()#14643
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:console-standard-fixes

Conversation

jasnell commented Aug 5, 2017

Copy link
Copy Markdown
Member

Per the console spec, the label in console.time() is a string. Change is made for consistency with browser behavior.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

console

console: coerce label to string in console.time()
d2ad13c 
Per the console spec, the label in console.time() is a string.
nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Aug 5, 2017
console: use default value for console.time()
7488fde 
Per the console spec, the default value of label is `'default'`.
refack mentioned this pull request Aug 5, 2017
Closed
3 tasks
refack approved these changes Aug 5, 2017
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

1 nit

Comment thread lib/console.js

Console.prototype.time = function time(label) {
Console.prototype.time = function time(label = 'default') {
label = `${label}`;

refack Aug 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why not simply String(label)?

lpinca Aug 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@refack I guess it's because String(Symbol('s')) does not throw.

refack Aug 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What a weird language 😖

refack Aug 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Found the piece I was missing, the console IDL - https://console.spec.whatwg.org/#console-namespace
@jasnell could you find a suitable place to reference that in the code?

TimothyGu Aug 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yep, IDL rules are weird. Before ES6 you could also use label + '' but not anymore due to some @@toPrimitive semantics that wouldn't work with Dates.

XadillaX Aug 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe we can write a line of comment to explain this? Or maybe next time who see it will be confused again.

refack reviewed Aug 5, 2017
View reviewed changes
Comment thread lib/console.js


Console.prototype.time = function time(label) {
Console.prototype.time = function time(label = 'default') {

refack Aug 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is okay because of the method binding in L62?

TimothyGu Aug 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What do you mean? It only binds the this value. And for the strictest compliance method.length === 0 because the IDL specifies label as an optional parameter.

refack Aug 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This would have changed the length unless it was bound, which some (@jasnell) consider semver-major.
It does change console.__proto__.time.length a.k.a. console.Console.prototype.time.length.

TimothyGu Aug 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

FWIW I do think this is semver-major, but because of the default change not .length.

XadillaX approved these changes Aug 6, 2017
View reviewed changes
Comment thread lib/console.js

Console.prototype.time = function time(label) {
Console.prototype.time = function time(label = 'default') {
label = `${label}`;

XadillaX Aug 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe we can write a line of comment to explain this? Or maybe next time who see it will be confused again.

tniessen approved these changes Aug 6, 2017
View reviewed changes
Comment thread lib/console.js
Console.prototype.time = function time(label) {
Console.prototype.time = function time(label = 'default') {
label = `${label}`;
this._times.set(label, process.hrtime());

tniessen Aug 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Personally, I would just use the template literal in this line and spare the assignment above.

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 8, 2017

jasnell commented Aug 8, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/9532/

jasnell commented Aug 8, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/9539/

jasnell added a commit that referenced this pull request Aug 8, 2017
console: coerce label to string in console.time()
4da8b99 
Per the console spec, the label in console.time() is a string.
Per the console spec, the default value of label is `'default'`.

PR-URL: #14643
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell commented Aug 8, 2017

Copy link
Copy Markdown
Member Author

Landed in 4da8b99

jasnell closed this Aug 8, 2017
jasnell mentioned this pull request Oct 30, 2017
Merged
niveditn added a commit to niveditn/node that referenced this pull request Nov 28, 2017
test: expanded assertions for console.timeEnd() output
a5ac0e3 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve nodejs#14544 but will
not address the whole thing.

Refs: nodejs#14643
niveditn mentioned this pull request Nov 28, 2017
Closed
3 tasks
addaleax pushed a commit that referenced this pull request Dec 1, 2017
test: expanded assertions for console.timeEnd() output
5282f96 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test: expanded assertions for console.timeEnd() output
efde4b7 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test: expanded assertions for console.timeEnd() output
ee1c95f 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca left review comments
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
BridgeAR BridgeAR approved these changes
+3 more reviewers
refack refack approved these changes
TimothyGu TimothyGu approved these changes
XadillaX XadillaX approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

console Issues and PRs related to the console subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL