Skip to content

Conversation

@mcollina
Copy link
Member

No description provided.

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

Still think that we should add a threshold and add more columns to the table :P

README.md Outdated
total.js | 2.9.4 | ✓ | 21019.2 | 4.65 | 6.03
trek-engine | 1.0.5 | ✗ | 28718.4 | 3.4 | 4.06
trek-router | 1.2.0 | ✓ | 28513.6 | 3.43 | 3.99

Copy link
Member

@aichholzer aichholzer Jun 12, 2018

Choose a reason for hiding this comment

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

@mcollina -Since this is not a markdown table, it looks pretty wonky...
Perhaps wrap it in ```, that should do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's what benchmarks.js compare -t outputs.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, but that's a "command line table", not copy-n-paste compatible with markdown. I can update the code to pump out a markdown table, if that's more what you intented.
It's easy though, just wrap that table in ``` and it will look perfectly pretty.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need something copy-pastable. I would prefer a markdown table, but I'll just do fine with ``` for now.

package.json Outdated
"ienoopen": "^1.0.0",
"inquirer": "^5.2.0",
"koa": "^2.5.0",
"inquirer": "^3.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

@mcollina -Is there a reason why some of the packages have been downgraded here? -Also, any particular reason as to why rayo was removed? -Might as well remove it from the results table if you don't see it fit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I probably messed up with the rebase, let me check.

@aichholzer aichholzer mentioned this pull request Jun 13, 2018
Copy link
Member

@aichholzer aichholzer left a comment

Choose a reason for hiding this comment

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

LGTM.
@delvedor & @mcollina -What's you merge PR policy? -Does the last reviewer (who approves) merge or does the author of the PR?

@mcollina
Copy link
Member Author

You can land this :). Thanks to taking it over the finishing line!

@aichholzer
Copy link
Member

No problem @mcollina -Anytime. It's my definition of being a team.

@aichholzer aichholzer merged commit 90b976d into master Jun 13, 2018
@aichholzer aichholzer deleted the updates branch June 13, 2018 11:14
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.

4 participants