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

refactor: rename request.validate to request.validateInput #4139

Merged
merged 7 commits into from Jul 19, 2022

Conversation

metcoder95
Copy link
Member

@metcoder95 metcoder95 commented Jul 14, 2022

Checklist

// or

const validate = request
.getSerializationFunction('body')
Copy link
Member

Choose a reason for hiding this comment

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

misprint: the serialization function

validate({ foo: 0.5 }) // false
```

See [.compilaValidationSchema(schema, [httpStatus])](#compileserializationschema)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
See [.compilaValidationSchema(schema, [httpStatus])](#compileserializationschema)
See [.compilaValidationSchema(schema, [httpStatus])](#compilevalidationschema)

This function will compile a validation schema and
return a function that can be used to validate data.
The function returned (a.k.a. _validation function_) is compiled
by using the provided `SchemaControler#ValidationCompiler`.
Copy link
Member

Choose a reason for hiding this comment

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

Should it be a link?

// or

const validate = request
.compileSerializationSchema({
Copy link
Member

Choose a reason for hiding this comment

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

misprint: the serialization is not involved here and it returns a string


Note that you should be careful when using this function, as it will cache
the compiled valiadtion functions based on the schema provided. If the
schemas provided is mutated or changed, the serialization functions will not
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
schemas provided is mutated or changed, the serialization functions will not
schemas provided are mutated or changed, the validation functions will not

the compiled valiadtion functions based on the schema provided. If the
schemas provided is mutated or changed, the serialization functions will not
detect that the schema has been altered and for instance it will reuse the
previously compiled serialization function, as the cache is based on
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
previously compiled serialization function, as the cache is based on
previously compiled validation function, as the cache is based on

a totally new schema (object), otherwise the implementation won't benefit from
the cache mechanism.

:Using the following schema as example:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:Using the following schema as example:
Using the following schema as an example:

const validate = request.compileValidationSchema(schema1)

// Later on...
schema1.properties.foo.type. = 'integer'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
schema1.properties.foo.type. = 'integer'
schema1.properties.foo.type = 'integer'

Copy link
Member Author

Choose a reason for hiding this comment

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

Note for myself: Do not write documentation late in the night 😅

validate({ hello: 'world' }) // false
```

Note that you should be careful when using this function, as it will cache
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just link a blog post (I volunteer to write it) to explain this.

I think it is too technical and the user may loose the focus on the "big picture"

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, didn’t find a way to make it less verbose. Actually I think a Blog post will explain pretty what’s about, happy to be part if I can be helpful somehow 🙂

// or

request
.serializeInput({ hello: 'world'}, 'query') // false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.serializeInput({ hello: 'world'}, 'query') // false
.validateInput({ hello: 'world'}, 'query') // false

@Eomm Eomm added the documentation Improvements or additions to documentation label Jul 16, 2022
docs/Reference/Reply.md Show resolved Hide resolved
docs/Reference/Reply.md Show resolved Hide resolved
}
}
}, 200)
validate({ hello: 'world' }) // false
Copy link
Member

Choose a reason for hiding this comment

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

What about renaming it to validateInput? (as serializeInput)

We did not land this feature yet

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I think it can follow the same pattern as with the serializeInput 👍

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

docs/Reference/Request.md Outdated Show resolved Hide resolved
docs/Reference/Request.md Outdated Show resolved Hide resolved
docs/Reference/Request.md Outdated Show resolved Hide resolved
docs/Reference/Request.md Outdated Show resolved Hide resolved
docs/Reference/Request.md Outdated Show resolved Hide resolved
docs/Reference/Request.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Jul 18, 2022
@metcoder95 metcoder95 changed the title docs: adjust documentation for new request/reply APIs refactor: rename request.validate to request.validateInput Jul 18, 2022
@metcoder95 metcoder95 requested review from Fdawgs and Eomm July 18, 2022 21:08
@metcoder95
Copy link
Member Author

@Eomm Adjusted the name of the method to validateInput, please have a look 🙂

@Fdawgs thanks a lot for the insights about the documentation, super helpful. I addressed them, let me know if there's any adjustment that needs to be done 👍

docs/Reference/Request.md Outdated Show resolved Hide resolved
@xtx1130
Copy link
Contributor

xtx1130 commented Jul 19, 2022

Why need to rename validate to validateInput? This is not mentioned in the pr.

Checklist

@Eomm
Copy link
Member

Eomm commented Jul 19, 2022

Why need to rename validate to validateInput? This is not mentioned in the pr.

This feature is not yet shipped, so we can change it.

I suggested it because the reply object has the new serializeInput function to avoid the collision with the .serialize one.
So I think it was better to name "equally" the new functions

Co-authored-by: 小菜 <xtx1130@gmail.com>
lib/request.js Show resolved Hide resolved
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

@mcollina
Copy link
Member

@climba03003 is this good to land?

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

It is a refactor on name and document update.

I believe it is good to go.

Copy link
Contributor

@xtx1130 xtx1130 left a comment

Choose a reason for hiding this comment

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

LGTM.
ref: #3970

@mcollina mcollina merged commit 0ca63a0 into fastify:main Jul 19, 2022
@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 Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants