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.

Sign up for GitHub

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

Jump to bottom

Add timing functionality #295

Merged
merged 3 commits into from Dec 5, 2021
Merged

Add timing functionality #295

merged 3 commits into from Dec 5, 2021

Conversation

eliasm307
Copy link
Contributor

Add timing functionality to resolve #291

@eliasm307 eliasm307 changed the title #291 Add timing functionality Add timing functionality Nov 16, 2021
@eliasm307 eliasm307 force-pushed the feat/add-timing-functionality branch 4 times, most recently from 0b52ec7 to 9f70e6a Compare November 16, 2021 02:37
@coveralls
Copy link

coveralls commented Nov 16, 2021 •
edited

Coverage Status

Coverage increased (+1.2%) to 99.807% when pulling 0794872 on eliasm307:feat/add-timing-functionality into 59de6e4 on open-cli-tools:master.

@gustavohenke gustavohenke self-requested a review November 18, 2021 11:12
gustavohenke
gustavohenke reviewed Nov 18, 2021
View reviewed changes
Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

whoaa this is super nice! I've added a bunch of thoughts, let me know what you think please.

There are some styling inconsistencies (but these aren't defined anywhere!), so I'll fix them up when we've agreed on the discussion below.

src/completion-listener.js Outdated Show resolved Hide resolved
src/flow-control/log-timings.js Outdated Show resolved Hide resolved
src/flow-control/log-timings.js Outdated Show resolved Hide resolved
src/flow-control/log-timings.js Outdated Show resolved Hide resolved
src/flow-control/log-timings.js Outdated Show resolved Hide resolved
bin/concurrently.js Outdated Show resolved Hide resolved
src/flow-control/log-timings.js Outdated Show resolved Hide resolved
@eliasm307 eliasm307 force-pushed the feat/add-timing-functionality branch 2 times, most recently from 18c911e to 3487f83 Compare November 18, 2021 21:47
@eliasm307
Copy link
Contributor Author

eliasm307 commented Nov 18, 2021 •
edited

@gustavohenke thanks for the feedback, I've applied it, let me know if you see anything else

@eliasm307 eliasm307 force-pushed the feat/add-timing-functionality branch 3 times, most recently from 6da8b2c to 596c9de Compare November 18, 2021 22:31
@eliasm307 @gustavohenke
Update src/flow-control/log-timings.js
7eb0499 
Co-authored-by: Gustavo Henke <guhenke@gmail.com>
@eliasm307
Copy link
Contributor Author

@gustavohenke the test coverage being almost 100% was annoying me so I've added a few tests to cover the remaining lines

gustavohenke
gustavohenke approved these changes Dec 5, 2021
View reviewed changes
Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, I'm just implementing a logger-friendly console.table and will push directly to your branch.
We need to respect a bunch of other combinations users can have (raw mode, using a custom stdout, etc)

@gustavohenke gustavohenke merged commit c04740a into open-cli-tools:master Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gustavohenke gustavohenke gustavohenke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option to show timings for each command in cli and also return timings when used programmatically
3 participants
@eliasm307 @coveralls @gustavohenke