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

Implementation of config parser validation and completion #80

Closed
wants to merge 31 commits into from

Conversation

mklauser
Copy link
Contributor

@mklauser mklauser commented Feb 3, 2014

This PR implements a new file that describes the config file with all options. Once such a file is read in it can be used to validate an actual config file. It completes default values and gives back a dict with all default values included and validated.

WIP

  • py.test readable tests
  • an actual default config
  • a documentation page.

OPTIONAL

  • conversion of a config validator to a documentation page

value_test:
integer:
property_type: int
default: 99.1
Copy link
Member

Choose a reason for hiding this comment

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

So the default is a non-int. but the type is int. What happens then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On 02/18/2014 07:42 PM, Wolfgang Kerzendorf wrote:

In tardis/io/default_conf_test/conf_def.yaml:

@@ -0,0 +1,74 @@
+value_test:

  • integer:
  •    property_type: int
    
  •    default: 99.1
    

So the default is a non-int. but the type is int. What happens then?


Reply to this email directly or view it on GitHub
https://github.com/tardis-sn/tardis/pull/80/files#r9835481.

Hi,
the following error should be raised
raise ValueError('Default value violates property constraint.')

Copy link
Member

Choose a reason for hiding this comment

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

so when would this happen? on readin? should this file be read in at all? anyways, to test this just include a little function:

def test_default_violates_property_constraint():
    with pytest.raises(ValueError):
        ...code that reads the wrong file

@wkerzendorf
Copy link
Member

@mklauser can you write a python test that runs your validator and tests the different values? there are some tests in tardis that you can check out. They should give you the general idea.

-sorry just realized that you have half of it implemented already.

mandatory: False
help: range for testing


Copy link
Member

Choose a reason for hiding this comment

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

I think the above tests are great! They sort of test the "leaf"-validation. Let's put the rest of the test in a separate file.

Copy link
Member

Choose a reason for hiding this comment

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

on second thought - I think just taking out the supernova section, makes this more logical.

@wkerzendorf
Copy link
Member

It looks really good! @ssim can you check out the conf_def.yaml file - that's how the validator will look like. I think we'll work on merging this PR (some testing and docu updates and then moving on to making the actual config definition).

@ssim
Copy link
Contributor

ssim commented Feb 19, 2014

I've taken a look at that file - looks good although I don't fully understand how this all slots together - you'll need to explain it to me sometime soon.

@wkerzendorf
Copy link
Member

@ssim, I'm gonna refer the explanation to @mklauser as he made it happen.

This was referenced Feb 20, 2014
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling bc8e50a on mklauser:default-parser into 56c9815 on tardis-sn:master.


tardis_config_version:
property_type: string
help: Version of the configuration file
Copy link
Member

Choose a reason for hiding this comment

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

is a default keyword required or is it optional - same question about mandatory. What I'm asking is there a default for default, mandatory and help. Either way is fine - should be noted in the documentation.

default: None
mandatory: True
help: file type
#### there are only a handful of types available how do we validate that ####
Copy link
Member

Choose a reason for hiding this comment

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

So I haven't seen the constraints in there. It was in one of the earlier versions. Anyways there is like 3 filetypes allower ['artis', 'tardis_simple', 'some other filetype'] how do I tell this here (and also how do I tell it must be >0, e.g.). Putting this in the documentation and tests will also help future users and us see the usage cases.



def default_parser_helper(test_dic, default, wdefault, value, wvalue, container, mandatory):
test_ob = DefaultParser(test_dic)
Copy link
Member

Choose a reason for hiding this comment

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

To me it is not quite clear what this function does. Especially the return n looks like some opcode. Anyways, I think you misunderstood me with the tests. They are very simple in python - Here's an example:

test = yaml.Load(file('conf_tes.yml'))

definition = yaml.Load(file('conf_def.yml'))
default_parser_obj = DefaultParsers(definition)

def test_integer():
    parsed_test = default_parser_obj.parse(test)
    assert parsed_test['int_value'] == int value that you suspect

does that make sense? We can chat over skype to clarify it if not.

@wkerzendorf
Copy link
Member

rebased at #132 - closing

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

4 participants