Adds schema methods to Option and Config classes. #646
Conversation
| """Returns the full json-schema for this Config instance.""" | ||
| schema = {'type': 'object'} | ||
| for name, instance in self.options.items(): | ||
| schema[name] = instance.schema() |
There was a problem hiding this comment.
How do you plan on handling default values and required options?
There was a problem hiding this comment.
I guess defaults might not be necessary as Config can already handle that.
There was a problem hiding this comment.
I was thinking about this in terms of the plugins, which already have Config classes that have defaults set for options.
One way to add 'required' options to the schema definitions might be to change the base Config class' schema to list all of its options under 'properties', and then have the required ones in a 'required' list:
{
'type': 'object',
'properties': {
'option1': { 'type': 'string' },
...
},
'required': ['option1']
}
There was a problem hiding this comment.
Actually while looking into 'required' properties I realize I need to nest all of the option schemas inside a 'properties' attribute, like in the comment on 'required' above. I will change this later today.
There was a problem hiding this comment.
To add to that, the tests should probably also try and load the schema with jsonschema.
There was a problem hiding this comment.
Also about the required: Should we have anything in required at all for a Config? Since we are allowing partial PATCH changes to a config.
There was a problem hiding this comment.
It feels like PATCHing is the special case. We'd want to check for required fields when creating a new Config.
| for name, instance in self.options.items(): | ||
| schema['properties'][name] = instance.schema() | ||
| if instance.is_required(): | ||
| required.append(name) |
There was a problem hiding this comment.
Why not add these to the schema directly?
There was a problem hiding this comment.
I originally tried having 'required': [] as part of the schema, and filling it as needed when options are required. But json-schema validate() throws exceptions if a schema it receives as input has an empty list of 'required'.
FranciscoCanas
force-pushed
the
jsonSchemaConfigs
branch
from
1081f50
to
8feed84
Compare
| @@ -30,6 +30,7 @@ | |||
|
|
|||
| class StringOption(Option): | |||
| """Represents a string value.""" | |||
| schema_type = 'string' | |||
There was a problem hiding this comment.
Since these are class constants, I would make them ALL_CAPS.
FranciscoCanas
force-pushed
the
jsonSchemaConfigs
branch
from
6428535
to
775fcf9
Compare
FranciscoCanas
changed the title
FranciscoCanas
force-pushed
the
jsonSchemaConfigs
branch
from
775fcf9
to
9307f29
Compare
Config schema methods generate a json schemas based on the config instance's options. Option schema methods generate schemas based on the type of Option, its default values, and whether its required or not. Also adds schema unit tests for Config and Options, and adds test_float unit test file. Needed for Freeseer#632
FranciscoCanas
force-pushed
the
jsonSchemaConfigs
branch
from
9307f29
to
2a128fc
Compare
| @@ -59,6 +61,7 @@ def decode(self, value): | |||
|
|
|||
| class FloatOption(Option): | |||
| """Represents a floating point number value.""" | |||
| SCHEMA_TYPE = 'number' | |||
There was a problem hiding this comment.
I haven't been following this project closely so perhaps I'm missing something, but would this be better as 'float'?
There was a problem hiding this comment.
The JSON schema spec doesn't actually include floats. Instead, it has 'number' for any numbers, both whole and fractional, and 'integer' for just integers. It's described here:
http://json-schema.org/latest/json-schema-core.html#anchor8
There was a problem hiding this comment.
Thanks, that's a handy page. Carry on.
Adds a schema() method to Config and Options classes that returns their json schemas. Adds unit tests to test_config.py to test schema methods for Config and all Option types. Also adds a test_float file with FloatOption unit tests.
Related to: #632