Skip to content

Edit grammar and syntax in readme #62

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

Merged
merged 2 commits into from
Dec 15, 2017

Conversation

bendrucker
Copy link
Contributor

Hey there, thanks for building this! Noticed a few sentences with awkward syntax or grammar in the readme. Went through and touched it up.

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

A clear part that we are missing here is that this should be combined with http://npm.im/flatstr.

README.md Outdated
__fast-json-stringify__ is two times faster than `JSON.stringify()`.
It is particularly suited if you are sending small JSON payloads, the
advantages reduces on large payloads.
__fast-json-stringify__ is significantly faster than `JSON.stringify()` for small payloads. Its performance advantage shrinks as your payload grows.
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain change a bit more? Have you experience a degradation in throughput?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to just rephrase the existing idea based on the benchmarks. The most significant change was removing "two times faster." I thought it was clearer to focus on the performance characteristics instead of using a rough average of the performance improvement that might not apply to many users.

Copy link
Member

Choose a reason for hiding this comment

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

👍 ok.

@bendrucker
Copy link
Contributor Author

A clear part that we are missing here is that this should be combined with http://npm.im/flatstr.

What do you mean?

@mcollina
Copy link
Member

See fastify/fastify#172.

@bendrucker
Copy link
Contributor Author

Gotcha, so you're suggesting that the readme should recommend using flatstr in combination with fast-json-stringify?

@mcollina
Copy link
Member

Yes.

@bendrucker
Copy link
Contributor Author

Added

@mcollina mcollina merged commit cd8886a into fastify:master Dec 15, 2017
@bendrucker bendrucker deleted the readme-typos-and-grammar branch December 15, 2017 22:32
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.

3 participants