Add default display of chart alongside expectations #33
Conversation
There was a problem hiding this comment.
@MickeyKay thanks. I left some comments. Can you also add some tests to that?
| @@ -20,7 +20,6 @@ class PWMetrics { | |||
| constructor(url, opts) { | |||
| this.url = url; | |||
| this.opts = opts; | |||
| this.opts.disableCpuThrottling = this.opts.disableCpuThrottling || false; | |||
There was a problem hiding this comment.
I don't think that we should remove this one.
There was a problem hiding this comment.
Restored. Accidentally removed this in my haste.
| @@ -76,6 +75,9 @@ class PWMetrics { | |||
| if (this.opts.json) { | |||
| return data; | |||
| } else if (this.opts.expectations) { | |||
| if (!this.opts.disableChart) { | |||
There was a problem hiding this comment.
What do you think about using showChart option and make it true by default?
There was a problem hiding this comment.
For sure - love it!
| @@ -21,6 +21,7 @@ class PWMetrics { | |||
| this.url = url; | |||
| this.opts = opts; | |||
| this.opts.disableCpuThrottling = this.opts.disableCpuThrottling || false; | |||
| this.opts.showChart = (typeof this.opts.showChart !== 'undefined') ? (this.opts.showChart == 'true') : true; | |||
There was a problem hiding this comment.
Not sure if this is the best way to do this, however it seems that setting --showChart=false in CLI passes the showChart option through as a string instead of a bool. I thought about automatically parsing this in cli.js, however I think this warrants a different PR altogether if you'd like to go in that direction. This tidbit forces a boolean value no matter what. Would love to hear if you have a better suggestion on how to handle this more gracefully.
There was a problem hiding this comment.
I see 2 possible solutions here
- easy one like you did, just simplify a bit:
this.opts.showChart = (this.opts.showChart === 'false') ? false : true; - manage it on
cli.jslevel - https://github.com/paulirish/pwmetrics/blob/master/bin/cli.js#L17
flags[flagKey] = keyValue[1] || true;
flags[flagKey] = (flags[flagKey] === 'false') ? false : flags[flagKey];
I'd prefer second one.
|
@denar90 Updated per your suggestions. I posed one question above and would love to hear your thoughts. |
| @@ -76,6 +77,9 @@ class PWMetrics { | |||
| if (this.opts.json) { | |||
| return data; | |||
| } else if (this.opts.expectations) { | |||
| if (this.opts.showChart) { | |||
| this.showChart(data); | |||
| } | |||
| expectations.checkExpectations(data.timings, this.opts.metrics) | |||
There was a problem hiding this comment.
Just one snit pick. @MickeyKay what do you think if we draw chart after expectations will be checked. Like
expectations.checkExpectations(data.timings, this.opts.metrics);
if (this.opts.showChart) this.showChart(data);
There was a problem hiding this comment.
@denar90 I actually prefer the existing arrangement, and here's why. Whenever I'm running any script, the final thing I always want to see in the output are errors/warnings. Reason being is that it's easier to lose track of said errors/warnings if they're earlier on in such output, and much less likely lost if they are the last piece of output.
Also, it seems to me like the logical order of things is 1. see timings 2. see errors/warnings resulting from these timings.
With that said, my preference would be to keep as is. Thoughts?
|
what if we just always show the chart, even if we're running against expectations? |
|
@paulirish this was my initial preference, hence the original (optional) |
|
@MickeyKay @paulirish SGTM. |
MickeyKay
force-pushed
the
feature/allow-chart-display-with-expectations
branch
from
2f3ad8c
to
feb3ce6
Compare
|
@denar90 @paulirish Okay, after all is said and done this PR is now super simple and simply outputs the chart regardless. |
|
@MickeyKay thanks! |
|
Oh wow. Merged already! Thanks 👍 |
Addresses #32
This PR enables default display of the chart when
expectationsare present, and adds a newdisableChartoption that can be used to output expectation errors and warnings alone.