Skip to content
This repository was archived by the owner on Jul 18, 2022. It is now read-only.

Fix validate twice #6

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix validate twice #6

wants to merge 7 commits into from

Conversation

nezuo
Copy link
Contributor

@nezuo nezuo commented Apr 18, 2021

The data is already validated in Document:readyPromise().

@evaera
Copy link
Owner

evaera commented Apr 19, 2021

If we're going to remove one, I think we should remove the one in readyPromise. This feels like the one that would be reused more, so we should make sure it's always validated here.

@nezuo
Copy link
Contributor Author

nezuo commented Apr 19, 2021

The reason I kept the one in readyPromise was because it has some custom error logic as apposed to just an assert. Should I move that error to the other location?

@evaera
Copy link
Owner

evaera commented Apr 19, 2021

Can you make the error in DocumentData an if statement instead of an assert, and then error() with

error(Error.new({
					kind = Error.Kind.SchemaValidationFailed,
					error = schemaError,
					context = ("Schema validation failed when loading data in collection %q key %q"):format(
						self.collection.name,
						self.name
					)
				}))

@nezuo nezuo requested a review from evaera April 24, 2021 00:10
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.

2 participants