Skip to content

Conversation

@delvedor
Copy link
Member

@delvedor delvedor commented Oct 4, 2016

Added support for additionalProperties.
See discussion in #5.

Ps: I took the occasion to add a table of contents to the docs.

@mcollina
Copy link
Member

mcollina commented Oct 4, 2016

We should add a case for additionalProperties: true and additionalProperties: false. If additionalProperties: true is passed, we use fast-safe-stringify to JSONify them (to avoid circular references).

@delvedor
Copy link
Member Author

delvedor commented Oct 4, 2016

In case of additionalProperties: false all the values that are not present in properties/patternProperties are ignored.

Regarding additionalProperties: true, I never see in the json docs this case, only false or object. That's why I wrote this.

@mcollina
Copy link
Member

mcollina commented Oct 4, 2016

Look at this: https://spacetelescope.github.io/understanding-json-schema/reference/object.html.

The additionalProperties keyword is used to control the handling of extra stuff, that is, properties whose names are not listed in the properties keyword. By default any additional properties are allowed.

I think we should "bend the rule here", and support additionalProperties  as being false by default. If someone wants to add random properties, I think we should support additionalProperties: true.

@delvedor
Copy link
Member Author

delvedor commented Oct 4, 2016

For me is ok support additionalProperties: true, but how?
You say to use fast-safe-stringify, but we can't use require in the generated code.
I saw the source code of fast-safe-stringify and it's composed by three different functions and it's exposing only one of them, so I can't fast-safe-stringify.toString() as we do with the other functions.

Ideas?

@mcollina
Copy link
Member

mcollina commented Oct 4, 2016

You can pass it in as an argument of https://github.com/mcollina/fast-json-stringify/blob/master/index.js#L55.

@delvedor
Copy link
Member Author

delvedor commented Oct 4, 2016

If this implementation is ok for you I'll update the docs to describe this behaviour :)

@mcollina
Copy link
Member

mcollina commented Oct 4, 2016

The implementation is ok.

@mcollina
Copy link
Member

mcollina commented Oct 4, 2016

Does this slows down benchmarks? I don't think so, but please check.

Anyway, LGTM.

@delvedor
Copy link
Member Author

delvedor commented Oct 4, 2016

I'll add more benchmarks in the future.
In the meantime I merge this :P

@delvedor delvedor merged commit 89e7387 into master Oct 4, 2016
@delvedor delvedor deleted the additional-properties branch October 4, 2016 19:34
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