Skip to content

Conversation

tech4him1
Copy link
Contributor

@tech4him1 tech4him1 commented May 17, 2018

- Summary

Closes #487.

I was just thinking about the best method for config validation, and this seemed like a viable option. @Benaiah's validator may be better, but I think it's worth a comparison.

This kind of basic format definitely does have the downside of only being able to statically check things. Most logic is available, though, and we can import constants from other files (valid formats, for example).

- Test plan

- Description for the changelog

Validate config with schema.

- A picture of a cute animal (not mandatory but encouraged)

@verythorough
Copy link
Contributor

verythorough commented May 17, 2018

Deploy preview for netlify-cms-www ready!

Built with commit ad12707

https://deploy-preview-1363--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented May 17, 2018

Deploy preview for cms-demo ready!

Built with commit ad12707

https://deploy-preview-1363--cms-demo.netlify.com

@tech4him1
Copy link
Contributor Author

tech4him1 commented Jul 1, 2018

I haven't updated the entire schema yet (this is still a WIP), but I've added all the validations we were currently doing in-code.

I think most error messages are clear enough, but for more complicated stuff like enum, oneOf, and if/then the correct options aren't actually listed.

Example error messages. I'd like opinions on which of these are unclear and need updated.

  • 'slug.encoding' should be equal to one of the allowed values
    • Should we list the allowed values in the error?
  • 'collections[0]' must have a field that is a valid entry identifier
    • This is a custom error message -- the default was incredibly unclear due to how I had to validate it in the schema.
  • config should have required property 'backend'
  • 'backend' should have required property 'name'
  • 'backend.name' should be string
  • config should have required property 'media_folder'
  • 'media_folder' should be string
  • config should have required property 'collections'
  • 'collections' should be array
  • 'collections' should NOT have less than 1 items
  • 'collections[0]' should be object
    • This is in the case where a collection is not an object (null, a string, etc.).

@tech4him1 tech4him1 requested a review from talves July 1, 2018 23:59
@erquhart
Copy link
Contributor

erquhart commented Jul 2, 2018

I think these are fine for a start, we can always improve moving forward.

@tech4him1 tech4him1 force-pushed the config-schema branch 3 times, most recently from 1c8393a to 8e34db6 Compare July 3, 2018 15:06
@tech4him1 tech4him1 changed the title WIP: Use config schema definition for validation. Use config schema definition for validation. Jul 5, 2018
@tech4him1 tech4him1 requested a review from erquhart July 5, 2018 20:29
@tech4him1
Copy link
Contributor Author

@erquhart Can you review this on a "real" config file (e.g. Netlify, Smashing, etc.)?

@tech4him1 tech4him1 requested a review from Benaiah July 6, 2018 20:32
@tech4him1
Copy link
Contributor Author

@Benaiah Any thoughts on this?

@jake-101
Copy link
Contributor

jake-101 commented Jul 9, 2018

i can't wait for this!!

@tech4him1
Copy link
Contributor Author

@erquhart @Benaiah @talves I think this should go in v2.0 if we are going for it -- that will avert any possible breaking changes caused by unintended behiviour that used to work.

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

@tech4him1 this is awesome - just one change needed, otherwise LGTM. I might go ahead and rebase since it's my fault it's behind lol.

widget: { type: "string" },
required: { type: "boolean" },
},
required: ["name", "label", "widget"],
Copy link
Contributor

Choose a reason for hiding this comment

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

widget isn't required, it's string by default.

},
},
},
required: ["name", "label"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we drop label too since that is a pending PR?

widget: { type: "string" },
required: { type: "boolean" },
},
required: ["name", "label"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we drop label too since that is a pending PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, makes sense - I'm pushing another commit right now, will include that.

@erquhart
Copy link
Contributor

erquhart commented Aug 3, 2018

CI will fail until I merge the new stuff from #1573.

@erquhart erquhart merged commit 576d23d into decaporg:master Aug 7, 2018
@tech4him1 tech4him1 deleted the config-schema branch August 7, 2018 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message for config issues
4 participants