Skip to content

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

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.

@eliasm307 eliasm307 force-pushed the feat/add-timing-functionality branch from de60210 to fbda2f7 Compare November 16, 2021 19:46
@eliasm307 eliasm307 force-pushed the feat/add-timing-functionality branch from fbda2f7 to 2bc5bcc Compare November 16, 2021 20:29
@gustavohenke gustavohenke self-requested a review November 18, 2021 11:12
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.

@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

@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 eliasm307 force-pushed the feat/add-timing-functionality branch from 596c9de to 7eb0499 Compare November 18, 2021 22:46
@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

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

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