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

Implement default key #88

Merged
merged 2 commits into from
May 1, 2018
Merged

Conversation

allevo
Copy link
Member

@allevo allevo commented Apr 30, 2018

Implement #87

The jsonschema can define default is the property is not given. This PR implements that feature.

@allevo allevo requested a review from delvedor April 30, 2018 20:21
const output = stringify(toStringify)
const outputUglify = stringifyUgly(toStringify)

t.deepEqual(output, JSON.stringify(expected))
Copy link
Member

Choose a reason for hiding this comment

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

I would use strictEqual here since we are working with strings.

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.

Can you also run a benchmark?

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.

LGTM

@allevo
Copy link
Member Author

allevo commented May 1, 2018

@delvedor

$ git checkout master 
Switched to branch 'master'
$ npm run benchmark
> node bench.js

JSON.stringify array x 2,425 ops/sec ±3.44% (85 runs sampled)
fast-json-stringify array x 2,941 ops/sec ±1.21% (87 runs sampled)
fast-json-stringify-uglified array x 2,911 ops/sec ±1.25% (89 runs sampled)
JSON.stringify long string x 9,423 ops/sec ±2.61% (83 runs sampled)
fast-json-stringify long string x 9,810 ops/sec ±0.95% (89 runs sampled)
fast-json-stringify-uglified long string x 9,533 ops/sec ±1.75% (89 runs sampled)
JSON.stringify short string x 3,797,522 ops/sec ±2.32% (90 runs sampled)
fast-json-stringify short string x 7,643,835 ops/sec ±0.83% (92 runs sampled)
fast-json-stringify-uglified short string x 7,599,888 ops/sec ±1.28% (91 runs sampled)
JSON.stringify obj x 1,334,082 ops/sec ±1.04% (89 runs sampled)
fast-json-stringify obj x 2,788,622 ops/sec ±1.38% (89 runs sampled)
fast-json-stringify-uglified obj x 2,752,812 ops/sec ±1.38% (88 runs sampled)
$ git checkout feature/implement-defaults 
Switched to branch 'feature/implement-defaults'
$ npm run benchmark
> node bench.js

JSON.stringify array x 2,523 ops/sec ±1.52% (88 runs sampled)
fast-json-stringify array x 2,953 ops/sec ±2.30% (87 runs sampled)
fast-json-stringify-uglified array x 2,916 ops/sec ±3.07% (87 runs sampled)
JSON.stringify long string x 9,673 ops/sec ±1.48% (91 runs sampled)
fast-json-stringify long string x 9,744 ops/sec ±1.11% (90 runs sampled)
fast-json-stringify-uglified long string x 9,750 ops/sec ±1.23% (89 runs sampled)
JSON.stringify short string x 3,742,537 ops/sec ±2.05% (88 runs sampled)
fast-json-stringify short string x 7,290,348 ops/sec ±5.00% (86 runs sampled)
fast-json-stringify-uglified short string x 7,645,764 ops/sec ±0.85% (91 runs sampled)
JSON.stringify obj x 1,326,023 ops/sec ±1.27% (88 runs sampled)
fast-json-stringify obj x 2,769,480 ops/sec ±1.59% (88 runs sampled)
fast-json-stringify-uglified obj x 2,790,791 ops/sec ±1.75% (89 runs sampled)

No changes

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

@delvedor delvedor merged commit 71d8f1e into fastify:master May 1, 2018
@delvedor
Copy link
Member

delvedor commented May 1, 2018

I've merged too quickly :D
Can you open another pr to update the docs?

@allevo allevo deleted the feature/implement-defaults branch May 1, 2018 09:48
@allevo allevo mentioned this pull request May 1, 2018
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.

None yet

3 participants