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

doc: document JavaScript tool for benchmark comparison #39835

Closed
wants to merge 3 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Aug 21, 2021

This makes it simpler for everyone to do the statistical analysis since
it doesn't require to install R and its dependencies.

Refs: https://github.com/targos/node-benchmark-compare

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 21, 2021
This makes it simpler for everyone to do the statistical analysis since
it doesn't require to install R and its dependencies.

Refs: https://github.com/targos/node-benchmark-compare
@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 25, 2021

```console
$ cat compare-pr-5134.csv | Rscript benchmark/compare.R
$ node-benchmark-compare compare-pr-5134.csv # or cat compare-pr-5134.csv | Rscript benchmark/compare.R

Choose a reason for hiding this comment

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

We can use npx tool to run global dependencies without installing.

Suggested change
$ node-benchmark-compare compare-pr-5134.csv # or cat compare-pr-5134.csv | Rscript benchmark/compare.R
$ npx node-benchmark-compare compare-pr-5134.csv # or cat compare-pr-5134.csv | Rscript benchmark/compare.R

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we can but the user was instructed earlier to install the dependency

Choose a reason for hiding this comment

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

IMHO, this step should be removed to simplify user workflow

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

jasnell pushed a commit that referenced this pull request Aug 28, 2021
This makes it simpler for everyone to do the statistical analysis since
it doesn't require to install R and its dependencies.

Refs: https://github.com/targos/node-benchmark-compare

PR-URL: #39835
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 28, 2021

Landed in 449147e

@jasnell jasnell closed this Aug 28, 2021
@targos targos deleted the targos-patch-2 branch August 28, 2021 17:10
targos added a commit that referenced this pull request Sep 6, 2021
This makes it simpler for everyone to do the statistical analysis since
it doesn't require to install R and its dependencies.

Refs: https://github.com/targos/node-benchmark-compare

PR-URL: #39835
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit that referenced this pull request Sep 6, 2021
This makes it simpler for everyone to do the statistical analysis since
it doesn't require to install R and its dependencies.

Refs: https://github.com/targos/node-benchmark-compare

PR-URL: #39835
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants