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

Docs/fluent schema #1485

Merged
merged 6 commits into from Mar 5, 2019
Merged

Docs/fluent schema #1485

merged 6 commits into from Mar 5, 2019

Conversation

Eomm
Copy link
Member

@Eomm Eomm commented Feb 25, 2019

Closes #1329
I open this PR to get some feedback for this document and what examples I could add more.

Note that this line:

const bodyJsonSchema = { ...S.object().valueOf(), vacation: 'sharedAddress#' }

is necessary because the JSON schema standard (and fluent-schema too) doesn't provide support for a JSON like this:

{ "my-key": "sharedSchemaId#" }

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

@mcollina
Copy link
Member

I truly don't like:

const bodyJsonSchema = { ...S.object().valueOf(), vacation: 'sharedAddress#' }

Is there a way around this? The reason why this is needed must be in the docs.

Can you add some unit tests for the examples?

@Eomm
Copy link
Member Author

Eomm commented Feb 26, 2019

Is there a way around this?

It could be changed to a simply JSON:

{
    type: 'object',
    properties: {
      vacation: 'sharedAddress#'
    }
}

The reason why this is needed must be in the docs.

Could I add a reference to Validation-and-Serialization.md?

Can you add some unit tests for the examples?

Sure

@mcollina
Copy link
Member

It could be changed to a simply JSON:

THat's better yes.

Could I add a reference to Validation-and-Serialization.md?

yes of course.

@Eomm
Copy link
Member Author

Eomm commented Feb 26, 2019

@mcollina It seems that fluent-schema doesn't work with node 6. here the output
I will open an issue to fastify-schema to notify for this behaviour.

What do you suggest to solve?

@mcollina
Copy link
Member

skip the tests on Node 6.

@Eomm
Copy link
Member Author

Eomm commented Feb 26, 2019

skip the tests on Node 6.

Simple but effective 😁
I didn't saw this technique before 👏

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

Copy link
Contributor

@aboutlo aboutlo left a comment

Choose a reason for hiding this comment

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

Good stuff here @Eomm! A couple of comments

docs/Fluent-Schema.md Outdated Show resolved Hide resolved
docs/Fluent-Schema.md Outdated Show resolved Hide resolved
Copy link
Contributor

@aboutlo aboutlo left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit 29a77b9 into fastify:master Mar 5, 2019
@Eomm Eomm deleted the docs/fluent-schema branch March 5, 2019 11:44
@delvedor delvedor added documentation Improvements or additions to documentation test Issue or pr related to our testing infrastructure. labels Mar 6, 2019
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation test Issue or pr related to our testing infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants