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

Config validation #89

Merged
merged 4 commits into from
Mar 24, 2017
Merged

Conversation

nbardy
Copy link
Contributor

@nbardy nbardy commented Feb 28, 2017

Added some tests and testing, but it could definitely use a little more.

The code has some inconsistencies between using nil values and missing keys in options maps. The schema could be made clearer if this was standardized.

#88

Copy link
Member

@lacarmen lacarmen left a comment

Choose a reason for hiding this comment

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

This looks like a great start! Two things I'd like to change:

  1. Let's move all the schema definitions into a separate namespace. I'd like to keep the noise out of the compiler namespace as it's already so length as it is.
  2. {Any Any} will have to be added to the main Config schema and the MetaData schema since users can any parameter to these maps to have it available in their templates. (The relevant PR is here)

@lacarmen
Copy link
Member

Comments not related to the code review:

Another thing that I would change is to have separate MetaData schemas for pages and posts since not all keys are shared between them (home? and navbar?, for example). However, this requires reworking how post/page content is read so I'm okay with leaving this one for later.

I agree there are some weird inconsistencies with how nils/missing keys are dealt with. These ones I can consolidate on my own.

@jerger
Copy link
Contributor

jerger commented Feb 28, 2017

Great work :-)
Have you ever looked at (schema/defn ^:always-validate ...) ?

This would allow to validate parameters & result of a function - you will need no additional (schema/validate ...).

Usage is:

(schema/defn ^:always-validate 
    fn-name :- result-schema
    "doc-string"
    [param1 :- param1-schema
     ...]
) 

@lacarmen
Copy link
Member

Hey @nbardy are you still working on this?

@lacarmen
Copy link
Member

Going to merge this to keep things moving along. I'll make the requested changes myself.

@lacarmen lacarmen merged commit f0a45b1 into cryogen-project:master Mar 24, 2017
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.

None yet

3 participants