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

console: timeEnd() doesn't throw an error if no matching label #5901

Closed
wants to merge 1 commit into from

Conversation

@ghaiklor
Copy link
Member

@ghaiklor ghaiklor commented Mar 25, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

console

Description of change

Based on #3514 discussion, I've removed throw an error in timeEnd() method, if there is no matching label.

@cjihrig
cjihrig reviewed Mar 25, 2016
View changes
lib/console.js Outdated
if (!stdout || typeof stdout.write !== 'function') {
throw new TypeError('Console expects a writable stream instance');
}
if (!(this instanceof Console)) return new Console(stdout, stderr);

This comment has been minimized.

@cjihrig

cjihrig Mar 25, 2016
Contributor

Please revert these style changes.

This comment has been minimized.

@ghaiklor

ghaiklor Mar 25, 2016
Author Member

@cjihrig What style guides do you using? Should I revert all the style changes or only this one?

This comment has been minimized.

@cjihrig

cjihrig Mar 25, 2016
Contributor

We have an ESLint setup. But, in general, you should change the bare minimum in your PRs. Otherwise, it makes tools like git blame harder to use.

@cjihrig
cjihrig reviewed Mar 25, 2016
View changes
lib/console.js Outdated

Console.prototype.timeEnd = function(label) {
var time = this._times.get(label);
if (!time) {

This comment has been minimized.

@cjihrig

cjihrig Mar 25, 2016
Contributor

It looks like this is the only thing in this file that needs to change. Can you revert all of the other changes.

This comment has been minimized.

@ghaiklor

ghaiklor Mar 25, 2016
Author Member

@cjihrig I see. Better don't touch styles at all.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Mar 25, 2016

@cjihrig done

@mscdex mscdex added the console label Mar 25, 2016
@cjihrig
cjihrig reviewed Mar 25, 2016
View changes
test/parallel/test-console.js Outdated
@@ -8,7 +8,8 @@ assert.ok(process.stderr.writable);
assert.equal('number', typeof process.stdout.fd);
assert.equal('number', typeof process.stderr.fd);

assert.throws(function() {
assert.doesNotThrow(function() {
console.time('some label');

This comment has been minimized.

@cjihrig

cjihrig Mar 25, 2016
Contributor

This is unnecessary.

@cjihrig
cjihrig reviewed Mar 25, 2016
View changes
lib/console.js Outdated
@@ -64,10 +64,7 @@ Console.prototype.time = function(label) {


Console.prototype.timeEnd = function(label) {
var time = this._times.get(label);
if (!time) {

This comment has been minimized.

@cjihrig

cjihrig Mar 25, 2016
Contributor

This check still needs to be here. And if the entry isn't there, you'd need to return.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Mar 25, 2016

@cjihrig got it, so we can ignore non-existing label.

@cjihrig
cjihrig reviewed Mar 25, 2016
View changes
lib/console.js Outdated
throw new Error('No such label: ' + label);
}
const time = this._times.get(label);
if (!time) return;

This comment has been minimized.

@cjihrig

cjihrig Mar 25, 2016
Contributor

Can you just replace the old throw with return;. It just needs to be a one line change.

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 25, 2016

I think so (see #3514 (comment)). What you had before was wrong. It would print something but it had the incorrect times.

@@ -8,10 +8,6 @@ assert.ok(process.stderr.writable);
assert.equal('number', typeof process.stdout.fd);
assert.equal('number', typeof process.stderr.fd);

assert.throws(function() {

This comment has been minimized.

@cjihrig

cjihrig Mar 25, 2016
Contributor

I didn't mean to delete the whole test. Just the one line that I commented on was unnecessary.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Mar 25, 2016

@cjihrig here is my code:

var console = require('console');

console.time('test');
console.timeEnd('no such label');
console.timeEnd('test');

Prints only for test label:

./node test.js
test: 0.652ms

The following code:

var console = require('console');
console.timeEnd('no such label');

prints nothing.

@cjihrig
cjihrig reviewed Mar 25, 2016
View changes
lib/console.js Outdated
}

This comment has been minimized.

@cjihrig

cjihrig Mar 25, 2016
Contributor

Please remove this newly added line.

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 25, 2016

LGTM with one comment.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Mar 25, 2016

@cjihrig I see now. I must change only lines where it actually needs. Updated.

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 25, 2016

@jasnell
Copy link
Member

@jasnell jasnell commented Mar 26, 2016

LGTM

@thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Mar 26, 2016

We are doing this so that we would be on par with browsers? It feels like what we are doing is correct. If there is a typo in the name or if the usage is wrong we explicitly throw and let the user know what is wrong.

@jasnell
Copy link
Member

@jasnell jasnell commented Mar 26, 2016

How about a compromise? Don't throw but emit a warning using process.emitWarning.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Mar 26, 2016

In case of wrong label you get nothing in the console. That way you will know that you forgot to close console.time(). Looks as expected behaviour for me.

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 18, 2016

@thefourtheye ... do you still object to this or ...?

@thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Apr 19, 2016

Yes, I am still not convinced that this change is better.

To think of it, emitting a warning, as you suggested, would be the right fit here. This is not a serious error, I agree, but there should be some feedback to the user.

@jasnell
jasnell reviewed Apr 19, 2016
View changes
lib/console.js Outdated
if (!time) {
throw new Error('No such label: ' + label);
return;

This comment has been minimized.

@jasnell

jasnell Apr 19, 2016
Member

@ghaiklor ... would it be possible for you to add a ...

process.emitWarning(`No such label: ${label}`);

...before returning here?

This comment has been minimized.

@thefourtheye

thefourtheye Apr 19, 2016
Contributor

Should we include console.timeEnd somewhere in the message?

This comment has been minimized.

@jasnell

jasnell Apr 19, 2016
Member

Wouldn't hurt... No such label '${label}' for console.timeEnd() perhaps? Keep in mind, also, that the warning includes a stack trace.

This comment has been minimized.

@thefourtheye

thefourtheye Apr 19, 2016
Contributor

Ah, then the message is just fine.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Apr 19, 2016

@jasnell @thefourtheye yeah, I will update branch in few hours.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Apr 19, 2016

Warning doesn't include a stack trace. So I updated message with @jasnell approach.

> console.timeEnd('no label');
> (node:51305) Warning: No such label 'no label' for console.timeEnd()
@jasnell
Copy link
Member

@jasnell jasnell commented Apr 19, 2016

@ghaiklor. It does, it's just suppressed in the default output. If you use the --trace-warnings flag it will print the stack trace. And if you watch for warnings using process.on('warning', function(warn) { /** ... **/ });, the warn object will have a stack property that returns the stack trace.

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 19, 2016

one additional suggestion: in the test case, you can add a check to make sure the warning is issued appropriately... something like...

process.on('warning', common.mustCall((warning) => {
  assert(/No such label 'no label' for console.timeEnd\(\)/.test(warning.message));
}));
@jasnell
Copy link
Member

@jasnell jasnell commented Apr 19, 2016

LGTM with a suggestion.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Apr 19, 2016

I've added check for warning event in the test case.

@thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Apr 19, 2016

LGTM

@jasnell
jasnell reviewed Apr 20, 2016
View changes
test/parallel/test-console.js Outdated
@@ -1,5 +1,5 @@
'use strict';
require('../common');
var common = require('../common');
var assert = require('assert');

This comment has been minimized.

@jasnell

jasnell Apr 20, 2016
Member

nit: these requires should use const

@jasnell
jasnell reviewed Apr 20, 2016
View changes
test/parallel/test-console.js Outdated
console.timeEnd('no such label');

process.nextTick(() => process.removeListener('warning', onWarning));

This comment has been minimized.

@jasnell

jasnell Apr 20, 2016
Member

just use process.once('warning', onWarning) above and this isn't necessary.

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 20, 2016

couple more nits after adding the warning check

@cjihrig
cjihrig reviewed Apr 20, 2016
View changes
test/parallel/test-console.js Outdated
assert.throws(function() {
assert.doesNotThrow(function() {
const message = /No such label 'no such label' for console\.timeEnd\(\)/;
const onWarning = common.mustCall(warning => assert(message.test(warning.message)));

This comment has been minimized.

@cjihrig

cjihrig Apr 20, 2016
Contributor

Does this pass linting?

This comment has been minimized.

@evanlucas

evanlucas Apr 20, 2016
Member

It shouldn't. We added a rule where parens are explicitly required I though

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 20, 2016

Yea, still LGTM once the comments are addressed.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Apr 21, 2016

@cjihrig updated and fixed conflicts with master branch.

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 21, 2016

It would probably be simpler to use assert.strictEqual() instead of a regular expression in your test, but I don't think it needs to hold this us. Could you squash your changes down to a single commit?

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Apr 21, 2016

@cjihrig yes, I can, but I'm not sure if I can push them with force flag. As I understand, I need to do:

git rebase -i master

Squash all commits in one and then

git push --force origin fix/3514
@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 21, 2016

It's fine to force push your own branch. No one else is using it.

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Apr 21, 2016

@cjihrig done

@jasnell
jasnell reviewed Apr 21, 2016
View changes
test/parallel/test-console.js Outdated
const message = /No such label 'no such label' for console\.timeEnd\(\)/;
const onWarning = (warning) => assert(message.test(warning.message));
process.once('warning', common.mustCall(onWarning));

This comment has been minimized.

@jasnell

jasnell Apr 21, 2016
Member

this could likely be rewritten and simplified to just:

process.once('warning', common.mustCall((warning) => {
  assert(/no such label/.test(warning.message);
});

There's likely very little need to test the entire warning message. You just want to verify that the warning is being issued.

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 21, 2016

Just one last nit while I was looking it back over :-) Looking good!

When timeEnd() provided with label that doesn't exists
it emits warning in the console, so developer get know about it.
@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Apr 21, 2016

@jasnell done.

BTW, why do I need squash commits by myself if it can be done when merging into master (after rebase)?

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 21, 2016

@ghaiklor ... while it's possible for the person landing the PR to squash everything, it's often easier for those of us who do the landing to have the author of the PR squash them down (typically because it saves time). I personally don't mind squashing commits when I land things.

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 21, 2016

@ghaiklor
Copy link
Member Author

@ghaiklor ghaiklor commented Apr 21, 2016

@jasnell I see, thanks for explanation 😃
CI is green, my first contribution ^^

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 21, 2016

Awesome! Thank you for the contribution and for the patience! @cjihrig ... does this LGTY?

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 21, 2016

:shipit: (yes, LGTM)

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 21, 2016

Awesome. Given that this is a semver-major, let's give time for one more round of last call comments from @nodejs/ctc. If there are no objections by Monday I'll get this landed.

@jasnell jasnell added this to the 6.0.0 milestone Apr 22, 2016
jasnell added a commit that referenced this pull request Apr 25, 2016
When timeEnd() provided with label that doesn't exists
it emits warning in the console, so developer get know about it.

PR-URL: #5901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell
Copy link
Member

@jasnell jasnell commented Apr 25, 2016

Landed in 1c84579

@jasnell jasnell closed this Apr 25, 2016
joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
When timeEnd() provided with label that doesn't exists
it emits warning in the console, so developer get know about it.

PR-URL: nodejs#5901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@ghaiklor ghaiklor deleted the ghaiklor:fix/3514 branch Apr 25, 2016
jasnell added a commit that referenced this pull request Apr 26, 2016
When timeEnd() provided with label that doesn't exists
it emits warning in the console, so developer get know about it.

PR-URL: #5901
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.