WIP: schemas #66

CJ-Wright · 2017-12-12T22:26:30Z

No description provided.

CJ-Wright · 2017-12-14T17:55:30Z

Try this instead http://docs.python-cerberus.org/en/stable/index.html

scopatz · 2017-12-17T00:47:47Z

regolith/tests/test_validators.py

+from regolith.validators import validate_schema
+from regolith.schemas import schemas
+from regolith.exemplars import exemplars
+import pytest


PEP8 Third party imports go before package imports

scopatz · 2017-12-17T00:48:27Z

requirements/run.txt

Is this really a valid requirement?

I took inspiration for this from conda forge recipes.

That is crazy if it is.

We usually include python as a run requirement in conda recipes, right?

yes, that is true. I wonder about whether this affects pip, though maybe we don't care 🤷‍♂️

scopatz · 2017-12-17T00:53:16Z

regolith/validators.py

+        The sequential keys for the data being accessed, used for
+        printing/debuging errors
+    """
+    if isinstance(record, dict):


check collections.abc.Mapping, not dict. Same goes for below.

scopatz · 2017-12-17T00:54:31Z

regolith/validators.py

+                raise ValueError('{} is required in {}'.format(k, keys))
+            elif k in record and k in schema:
+                validate_schema(record[k], schema[k], keys + (k, ))
+    elif isinstance(record, collections.Iterable) and not isinstance(record,


Does this really include set and generators? Or is this supposed to be collections.abc.Sequence?

scopatz · 2017-12-17T00:55:02Z

regolith/validators.py

+            for r in record:
+                validate_schema(r, schema, keys)
+    else:
+        if not isinstance(record, schema['type']):


This should probably be an elif, rather than an if inside of an else.

scopatz · 2017-12-17T00:59:01Z

regolith/tests/test_validators.py

@@ -0,0 +1,11 @@
+from regolith.validators import validate_schema
+from regolith.schemas import schemas
+from regolith.exemplars import exemplars


If the exemplars are only used in the tests, then they should be in the test suite, not the package.

scopatz · 2017-12-17T01:01:19Z

regolith/schemas.py

@@ -0,0 +1,400 @@
+"""Database schemas"""
+schemas = {


PEP8 global constants should be in UPPERCASE

scopatz · 2017-12-17T01:02:39Z

regolith/validators.py

+        The database record to be tracked
+    schema : dict
+        The schema to validate the record against
+    keys : tuple


Should be listed as , optional

scopatz · 2017-12-17T01:03:49Z

regolith/validators.py

+
+        for k in total_keys:
+            if k not in schema:
+                pass


Isn't it an error if we get a key that we don't expect?

scopatz · 2017-12-17T01:07:31Z

regolith/validators.py

+        The schema to validate the record against
+    keys : tuple
+        The sequential keys for the data being accessed, used for
+        printing/debuging errors


Broadly speaking, I guess I'd prefer if this returned (bool, str message) rather than raising an error. That way the call site can decide whether or not to raise the error. Also, it allows the message to be appended to so that all of the ways that the record doesn't adhere to the schema can be known with one call to this function.

I think this is done with cerberus. (validator.errors)

scopatz · 2017-12-17T01:11:13Z

This is a really great first cut @CJ-Wright! Other than the parts listed inline, I think the thing that makes sense (so that info isn't duplicated) is to have the docs in conf.py generate the schema section of the collections pages from the schema itself. I think that this is needed so there aren't two, possibly diverging sources of truth.

scopatz · 2017-12-17T01:13:16Z

Also, cerebus is interesting, but it seems to not be on conda-forge.

CJ-Wright · 2017-12-17T01:13:54Z

https://github.com/conda-forge/cerberus-feedstock

scopatz · 2017-12-17T01:14:46Z

Ahh, mistyped it!

CJ-Wright · 2017-12-17T01:15:06Z

Sorry, I just pushed up a cerberus driven version

scopatz · 2017-12-17T01:19:17Z

regolith/validators.py

 import os
 import re
+
+from cerberus import Validator
 from getpass import getpass


PEP8 cerberus is a 3rd party library and so should go below getpass, which is from the standard library and should be grouped with those above.

scopatz · 2017-12-17T01:23:23Z

regolith/validators.py

+    if isinstance(schema, dict):
+        schema.pop('description', None)
+        for v in schema.values():
+            pop_description(v)


You shouldn't need to do this. Instead subclass validtor and make it skip custom keys in the schema we want to add.

See http://docs.python-cerberus.org/en/stable/customize.html#custom-validators

I spent some time trying to do that without much headway, but I will try again.

There has to be a way. Popping stuff out of a global, mutable object is wrong.

In this case at least. The alternative is to have a second dict which has the same structure and only contains the descriptions, but that feels wrong too.

Btw there is an open issue for description pyeve/cerberus#254

And now a SO question https://stackoverflow.com/questions/47851546/cerberus-additional-schema-keys

scopatz · 2017-12-17T01:23:58Z

regolith/tests/test_validators.py

+from regolith.validators import validate_schema
+from regolith.schemas import SCHEMAS
+from regolith.exemplars import EXEMPLARS
+import pytest


PEP8 import order

scopatz · 2017-12-18T20:59:16Z

regolith/schemas.py

+class MyValidator(Validator):
+    def _validate_description(self, description, field, value):
+        if False:
+            self._error(field, "Shouldn't be here")


Can't this function just be a pass?

scopatz · 2017-12-18T20:59:47Z

regolith/schemas.py

+    return v.validate(record), v.errors
+
+
+EXEMPLARS = {'abstracts': {'_id': 'Mouginot.Model',


PEP8 Constants should be at the top of the file.

…to gitignore

scopatz · 2017-12-18T21:24:46Z

regolith/fsclient.py

@@ -48,10 +49,14 @@ def dump_yaml(filename, docs, inst=None):
    """Dumps a dict of documents into a file."""
    inst = YAML() if inst is None else inst
    inst.indent(mapping=2, sequence=4, offset=2)
-    for doc in docs.values():
+    my_sorted_dict = ruamel.yaml.comments.CommentedMap()


please remove the my_ from this variable.

scopatz · 2017-12-18T21:26:44Z

docs/collections/index.rst

+    auto/people
+    auto/projects
+    auto/proposals
+    auto/students


I'd really prefer these not go into an auto folder. It doesn't seems necessary.

scopatz · 2017-12-18T21:27:06Z

docs/collections/index.rst

-    grades
-    grants
+    auto/grades
+    auto/grants
    jobs
    news


Why are these not generated?

I haven't made schemas for them yet

haha OK - should that be a requirement for this PR? Or do you want to delay it?

I was hoping to delay it. I'm expecting a couple more takes at this as I start using more of the dbs so I might come around to it.

Sounds good to me!

scopatz · 2017-12-18T21:30:21Z

regolith/schemas.py

+                          'email': '[email protected]',
+                          'university_id': 'HAP42'}}
+
+SCHEMAS = {'abstracts': {'_description': {'description': 'Abstracts for a conference or workshop. This is generally public information\n\n'},


many lines too long here, also probably shouldn't have \n\n at the end of them. That can be added in the doc generator

scopatz · 2018-01-02T20:53:16Z

regolith/app.py

@@ -6,6 +6,8 @@

 from flask import Flask, abort, request, render_template, redirect, url_for

+from regolith.schemas import validate
+


PEP8 two newlines required after import lines

scopatz · 2018-01-02T20:55:22Z

regolith/app.py

+                n = os.path.join(td.name, 'regolith.txt')
+                print(errors)
+                print('Writing text file to {}. '
+                      'Please try again.'.format(n))


I think these prints should go into the body of the error message below. At the very least, they should be printed to stderr....

scopatz · 2018-01-02T20:56:08Z

regolith/app.py

+                print(errors)
+                print('Writing text file to {}. '
+                      'Please try again.'.format(n))
+                with open(n) as f:


This file isn't opened in write mode. Do you have any tests for the invalid case?

I don't think we have any tests for the app.

Fair point!

Made an issue #69

scopatz · 2018-01-02T20:56:29Z

regolith/app.py

+                print('Writing text file to {}. '
+                      'Please try again.'.format(n))
+                with open(n) as f:
+                    f.write(form['body'])


Same as above

scopatz · 2018-01-02T20:58:47Z

regolith/tests/test_validators.py

@@ -0,0 +1,8 @@
+import pytest


Can you please move the tests files to a top-level tests/ dir, rather than embedding them in the project dir? Thanks!

scopatz · 2018-01-02T20:59:17Z

Thanks! Some more minor comments.

CJ-Wright · 2018-01-02T22:41:54Z

@scopatz ready for another round!

scopatz · 2018-01-02T22:50:10Z

Looks good to go! Can you please provide a news entry?

scopatz · 2018-01-02T23:20:45Z

news/schema.rst

@@ -0,0 +1,16 @@
+**Added:**
+
+* Schemas for the tables


This is not particularly informative. Can you please expand on it. Also, aren't we calling them collections, not tables?

scopatz · 2018-01-03T15:16:07Z

news/schema.rst

+required fields are filled and the values are the same type(s) listed in the
+schema. The schema also includes descriptions of the data to be included.
+The exemplars are examples which have all the specified fields and are
+used to check the validation.


This is not valid rst, which requires the paragraph to be indented after the bullet point.

I'll merge this and fix it though.

WIP: schemas

3bdb6aa

CJ-Wright requested a review from scopatz December 12, 2017 22:26

CJ-Wright added 2 commits December 12, 2017 20:01

TST: add test for validator

4c6ce36

ENH: working schemas with examples

4c2e92d

CJ-Wright self-assigned this Dec 13, 2017

CJ-Wright added 3 commits December 12, 2017 22:46

DOC: add docs

5c2dd0f

CI: create not install

c684b86

MTN: fix run and move the schemas out of folder

4a498b3

CJ-Wright added 2 commits December 14, 2017 17:09

ENH: validator

e2ae650

WIP: finalize schema

f8f7930

scopatz reviewed Dec 17, 2017

View reviewed changes

STY: pep8

2969e52

scopatz reviewed Dec 17, 2017

View reviewed changes

scopatz reviewed Dec 18, 2017

View reviewed changes

CJ-Wright added 7 commits December 18, 2017 15:59

DOC: remove auto docs, add to their own auto folder, add auto folder …

c026c1b

…to gitignore

FIX: comments from @scopatz

55d3619

ENH: move top docs to schemas module

1394df2

STY: pep8 line width

61ffe55

STY: pep8 newline

9c3c8d6

ENH: alph. yamls and added top docs to schema

79b75a9

STY: pep8 line width

6222199

scopatz reviewed Dec 18, 2017

View reviewed changes

FIX: comments from @scopatz

4420cb5

scopatz reviewed Jan 2, 2018

View reviewed changes

CJ-Wright added 2 commits January 2, 2018 16:31

FIX: fix app, move tests

b7d3565

FIX: move error to raise

1491d88

news

a990ed9

scopatz reviewed Jan 2, 2018

View reviewed changes

elaborate on news

5df3883

scopatz reviewed Jan 3, 2018

View reviewed changes

scopatz merged commit 5063ba7 into regro:master Jan 3, 2018

CJ-Wright deleted the schemas branch January 3, 2018 15:34

		return v.validate(record), v.errors


		EXEMPLARS = {'abstracts': {'_id': 'Mouginot.Model',

		@@ -6,6 +6,8 @@

		from flask import Flask, abort, request, render_template, redirect, url_for

		from regolith.schemas import validate

WIP: schemas #66

WIP: schemas #66

Conversation

CJ-Wright commented Dec 12, 2017

CJ-Wright commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scopatz commented Dec 17, 2017

scopatz commented Dec 17, 2017

CJ-Wright commented Dec 17, 2017

scopatz commented Dec 17, 2017

CJ-Wright commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scopatz commented Jan 2, 2018

CJ-Wright commented Jan 2, 2018

scopatz commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment