Widened type interface #374

bsmithers · 2017-11-27T21:04:02Z

Following on from discussions in #364 I think this is pretty much ready to be merged in, though of course let me know if there are any issues. I think the only bit that is really 'new' is the conversion from the old default_types & types to functions that are redefined in the TypeChecker

Rebased the changes off of master removing any use of Draft6 (squashed into one commit)
No changes to TypeChecker interface since latest discussions in the issue
Expanded test coverage. Since the JSON schema test suite tests will now use the new TypeChecker, I've added tests that re-run the types tests explicitly on the old interface.
Updated docs - hopefully is all clear.

The TypeChecker provides an immutable interface to a mapping of type names to functions that check a value. Includes test coverage for the new TypeChecker, including demonstrations that it is sufficient to extend types. The old type interface is still supported, but will raise a DeprecationWarning. The old isinstance checks are converted to functions & passed to the TypeChecker. The old types are also checked with the JSON schema test suite 'type' tests.

- Included description of the new TypeChecker class and autodocs, added new exception, marked old interface as deprecated, updated example on how to provide a custom type check. Couple of code changes from writing docs: - Renamed type_ -> type in TypeChecker method params as Sphinx isn't happy with these names - Marked the type_checkers attribute as private

Julian · 2017-11-29T01:10:51Z

Woohoo! Awesome.

Will leave some comments but probably will be a trickle until I get a full review done.

Really appreciated!

Julian · 2017-11-29T01:11:41Z

docs/creating.rst

-        of the validator class when mapping between JSON types to Python
-        types. The default for this argument is probably fine. Instances
-        can still have their types customized on a per-instance basis.
+    :argument dict default_types: Deprecated. Please use the type_checker


Might be nice to use a sphinx deprecated directive even though we haven't been very careful with versionchanged either...

type_checker -> ``type_checker`` might also be nice here.

Hah - I nearly mentioned that in the PR. I'll double check, but the deprecated directive appears to require a version that something was deprecated in and I wasn't sure of your approach to versioning & releases.

Yeah -- I think if it helps that you can assume the next version will be 2.7.0 (but lemme know if there's anything else making that harder).

Julian · 2017-11-29T01:14:08Z

docs/creating.rst

+        argument instead.
+
+        If set, it provides mappings of JSON types to Python types that will
+        be converted to functions and redefined in this object's TypeChecker


Period at the end please, and either "type checker" (i.e. not the class name) or ":class:TypeChecker" at the end.

Julian · 2017-11-29T01:14:37Z

docs/creating.rst

+        be converted to functions and redefined in this object's TypeChecker
+
+    :argument jsonschema.TypeChecker type_checker: an instance
+        of :class:`TypeChecker`, whose :meth:`is_type` will be called to


It won't insert the word "method", so you have to say ":meth:is_type method"

Julian · 2017-11-29T01:16:25Z

docs/creating.rst

+    :argument jsonschema.TypeChecker type_checker: an instance
+        of :class:`TypeChecker`, whose :meth:`is_type` will be called to
+        validate the :validator:`type` property If unprovided, a default
+        :class:`TypeChecker` will be created, with no support types.


I think actually we can leave off the "if unprovided" entirely here and just rely on people looking at the newer property (which we've already pointed them to) for info on default behavior.

Julian · 2017-11-29T01:18:12Z

docs/creating.rst

@@ -59,6 +65,10 @@ Creating or Extending Validator Classes

    :argument str version: a version for the new validator class

+    :argument jsonschema.TypeChecker type_checker: an instance
+        of :class:`TypeChecker`. If unprovided, the existing


I try not to repeat instance-of-class kind of info in the actual prose (because it's already in the :argument: role). It's easy to get around with just "a type checker. If unprovided, the existing :class:TypeChecker will be used.", even though the class name is pretty similar in this case.

Julian · 2017-11-29T01:19:22Z

docs/index.rst

@@ -47,6 +47,7 @@ Contents:
    references
    creating
    faq
+    types


Any reason to add this as an entirely new document? If not might just be reasonable to put this in the validate.rst doc, I think even format is already just in that same doc.

Julian · 2017-11-29T01:21:12Z

docs/types.rst

+Type Checking
+=============
+
+Each :class:`IValidator` has an associated :class:`TypeChecker`. The


So, half baked thought, but I guess in retrospect it's not even obvious that every validator has the type validator. In terms of JSON Schema it seems pretty likely that it will stay forever, but in the more generic notion, a bit less so... Maybe someone creates a validator that doesn't have that property (http://github.com/Julian/Seep is already such an example I think), in which case providing a type_checker to create is a bit random...

Definitely not saying to change anything, but feel the need to note that uncomfortableness somewhere.

Interesting use case. I agree that it seems pretty core to JSON schema. Nonetheless I've reworded the docs slightly. It is slightly clunky but one would simply not provide a TypeChecker in such a case.

Julian · 2017-11-29T01:21:37Z

docs/types.rst

+=============
+
+Each :class:`IValidator` has an associated :class:`TypeChecker`. The
+TypeChecker provides an immutable mapping between names of types and


Would recommend "type checker" again here.

Julian · 2017-11-29T01:23:26Z

docs/types.rst

+.. autoexception:: jsonschema.exceptions.UndefinedTypeCheck
+
+    Raised when trying to remove a type check that is not known to this
+    TypeChecker. Internally this is also raised when calling


We don't really get a choice on the "internally" I think.

TypeChecker.is_type is public, so everyone needs to know it raises this exception there too, so we can never really consider it internal anymore.

Julian · 2017-11-29T01:35:00Z

docs/validate.rst

@@ -48,8 +50,10 @@ classes should adhere to.

    .. attribute:: DEFAULT_TYPES

-        The default mapping of JSON types to Python types used when validating
-        :validator:`type` properties in JSON schemas.
+        Deprecated. Under normal usage, this will be an empty dictionary.


We can't make this an empty dictionary, it has to preserve the existing behavior at least for Draft3Validator and Draft4Validator, and also for anyone who is using the default_types argument to create. If tests are missing for that we probably need to add them, it's existing behavior that someone could be relying on, so we can deprecate it but can't change it until we remove all those arguments.

We also need to have accessing the attribute warn with one of the warnings you've added (thanks!)

We can either use a property for that, or look at one of the libraries that are focused around "deprecation" itself if it starts to get annoying.

Good catch. This is partly a wording issue but I'm also going to revisit that in the code. Currently default_types is set to the old default if no TypeChecker is provided. However, you made me realise that this always needs to be true, because of direct access to the property.

We also need to have accessing the attribute warn with one of the warnings you've added (thanks!)

This isn't quite a trivial as first sounds - DEFAULT_TYPES is a class property and access to it is very likely to be done so on the class rather than a concrete instance.

So we could do one of the fudges for class properties, but is it worth it? Any access to DEFAULT_TYPES is likely (and arguably only a problem) to be followed by some derived use (i.e. create(default_types=...) or validator(types=...)), both of which would trigger a deprecation warning.

You'd need a metaclass yeah (which was what I meant by "a deprecation library", though the one I was intending I could have sworn had something for this, but I don't see it)

Besides the fact that it's uncomfortable to try to guess what code exists (I've been surprised / horrified at some of the jsonschema-using-code I've seen :) ), it seems like someone who even sees the warning from one of those two places is not-totally-unlikely to only half fix things (by just continuing to use DEFAULT_TYPES and just passing that into the new interface).

So I'm not definitely convinced we need it, but yeah I'd lean towards it being worthwhile, despite being annoying :/ If it presents a real annoyance though let me know and we should find something that does this. It's one of the things that is super super frustrating about Python in 2017 -- it should have a good story for this but unfortunately still doesn't...

Julian · 2017-11-29T01:38:04Z

docs/validate.rst

-        ...
+        pass
+
+    def is_my_int(instance):


Question which maybe was more obvious when writing this: maybe these functions should take both the instance and also the checker itself, rather than needing to "manually" pull that off?

(E.g., that'd allow for more easily reusing functions across drafts)

I'm not sure I follow this one - seems it will end up quite circular as each of these functions is already attached to a checker. I also wonder if it would be a performance hit, since each type check would then also be doing a comparison on type checker object? Or maybe I misunderstood.

For reusing across drafts - that's already handled through redefinition of types->functions, no? As in, most of the type checks remain the same and a new draft only redefines a subset.

They're attached to a checker, but they don't know what the "current" checker knows.

E.g., imagine someone wants to write a checker that satisfies "number or string" -- they can't do that under the "current" definition of number of a type checker, whatever that might be after someone changes things. I.e., compare:

def number_or_string(instance): return isinstance(instance, int) or isinstance(instance, str)

vs.

def number_or_string(checker, instance): return checker.is_type(instance, "number") or checker.is_type(instance, "string")

(This is a toy example, but hopefully the point is clear enough now at least? If not lemme know)

Ah yeah I get it now. I think that's a worthwhile change. It's particularly useful for custom types as when you're overwriting the checking function for an existing type you'll no longer be able to access the original via checker.

Julian · 2017-11-29T01:38:19Z

docs/validate.rst

+        pass
+
+    def is_my_int(instance):
+        return Draft3Validator.TYPE_CHECKER.is_type(instance, "number") or \


Parens instead of \ please.

Julian · 2017-11-29T01:40:27Z

jsonschema/_types.py

+
+        type_checkers (pyrsistent.pmap):
+
+            It is recommend to set type checkers through


Hm, is there a reason to recommend that over the param to __init__? If there is, I think it'd be better to remove the possibility entirely and stick to just that. (But if we can safely support both, seems fine to do so and to drop the recommendation).

Yeah, this is kind-of an awkwardness with the attrs frozen class. In order for evolve to work, the attribute must be able to be set via init. This probably means exposing the underlying type (i.e. pyrsistent.pmap), which doesn't seem right.

I suppose that we could have init take in a standard dictionary instead, which is then converted to the pmap by the TypeChecker (and then a little juggling in the update methods). This doesn't feel massively clean either but is perhaps the preferred solution.

Thoughts? Sorry this didn't come up earlier, I overlooked this before as the init is generated by attrs.

Yeah so this is the juggle to make it work:

_type_checkers = attr.ib(default=()) def __attrs_post_init__(self): object.__setattr__(self, "_type_checkers", pyrsistent.pmap(dict(self._type_checkers)))

As it happens, this is the same as the attrs issue you open a few months back: python-attrs/attrs#207

I think this is probably the right solution, however. It means that checkers can be defined on init without users needing to supply a pmap. Given this, we could then drop the redefine_many method (since the Draft3 TypeChecker initialisation was the main internal use of this) but I'm inclined to keep it around.

Edit: redefine_many is likely also useful in converting the old type checking - further justification to keep it.

What's making you reluctant to ask for a pmap and expect callers to provide one out of curiosity?

(If you did have such a reservation, you can use the coerce argument to attr.ib, but my personal position happens to be that in 2017, essentially make believe the stdlib types don't exist :)

It feels like an implementation detail. To me, the TypeChecker should provide an immutable interface but the way in which it does so shouldn't need to be public. Secondarily, it's simple, familiar and more concise for the caller if they can provide a vanilla dict.

I'm not really too fussy, however.

(Thanks for the pointer, that's a much cleaner way of doing it - looks like it's convert to attr.ib)

Julian · 2017-11-29T01:40:57Z

jsonschema/_types.py

+
+        Arguments:
+
+            instance (any primitive type, i.e. str, number, bool):


Hm, it's not just primitive types is it? It's any JSON-deserialized thing?

Julian · 2017-11-29T01:41:35Z

jsonschema/_types.py

+        try:
+            return self._type_checkers[type](instance)
+        except KeyError:
+            raise UndefinedTypeCheck


Should include the name of the type here (and ideally add that to the test that checks this branch)

Julian · 2017-11-29T01:42:14Z

jsonschema/_types.py

+                if type is unknown to this object.
+        """
+        try:
+            return self._type_checkers[type](instance)


Pedantry: in theory someone can add a type checker that itself raises KeyError, so a bit safer to do this in 2 steps and only catch the first one (and might be worth adding a test for that even though it's an edge case).

Julian · 2017-11-29T01:43:46Z

jsonschema/_types.py

+
+        """
+        definitions = dict(definitions)
+        evolver = self._type_checkers.evolver()


Isn't this just self._type_checkers.update(definitions)?

Julian · 2017-11-29T01:45:27Z

jsonschema/compat.py

@@ -28,7 +28,7 @@
    )
    from urllib import unquote  # noqa
    from urllib2 import urlopen  # noqa
-    str_types = basestring
+    str_types = basestring,


Shouldn't need this to be a tuple, do you?

Actually no, I didn't need this to change in the end - at some point it was looking like I did.

Isn't it odd that str_types is a tuple in PY3 and not the PY2 though?

I mean, mostly it's isinstance that's the odd thing and accepts either types or tuples of types, and since this is essentially only used for that, it's "fine" to mix those types.

Doesn't matter too much one way or the other though.

Julian · 2017-11-29T01:45:39Z

jsonschema/exceptions.py

@@ -140,6 +140,9 @@ class RefResolutionError(Exception):
    pass


+class UndefinedTypeCheck(Exception):
+    pass
+


2 blank lines please.

Julian · 2017-11-29T01:46:42Z

Left a bunch of comments, most of which are nitpicks, still need to review the tests, but lemme know what you think.

bsmithers · 2017-11-29T15:32:47Z

Thanks for reviewing, I'll look at this fully when I get some time but I've left a couple of replies.

Converted to a pmap in attr's post init.

- Default_types is always available, in case of direct access. - Use a metaclass (provided by six for py2 & py3 compat) to set a classproperty on a Validator that raises a deprecation warning - Add test coverage to check deprecation warnings are raised when appropriate

This reverts commit 9c1c265.

- Cleaned than the post_init approach reverted in the previous commit

Julian · 2017-12-10T15:02:14Z

Hey! Should I be looking at this again yet?

bsmithers · 2017-12-10T15:12:51Z

Hey, sorry for the slow down on this, I haven't quite had time to finish things off. Mostly just need to review the documentation changes. I'll try to get that done in the next day or so.

I've switched to using convert for the type_checker init as per your suggestion and fixed the the deprecation of DEFAULT_TYPES - the attribute will always be available but any access will trigger a deprecation warning (added test coverage for this also).

Julian · 2017-12-10T15:51:20Z

Trusssst me I get the not having enough time part :) was just checking if you meant to resubmit it yet, but no rush at all.

Improves composability, esp for custom types.

Conflicts: jsonschema/validators.py A change to the way bools are checked which appears to be redundant in the new type checker interface.

bsmithers · 2017-12-11T15:09:35Z

Think this is ready for a second look.

One thing I'm unsure of it's what's happening with the bool checks and perf - I've seen there's been a few changes but I'm not exactly sure what's been going on or how the performance tests are setup (I think it's ad-hoc at the minute?)

Julian · 2017-12-11T18:21:45Z

Yeah unfortunately the perf stuff is still ad-hoc because I can't find a place that I can push them to yet.

Julian · 2017-12-24T15:49:50Z

jsonschema/validators.py

+           type_checker=None):
+
+    use_default_types=False
+    if default_types is not None or type_checker is None:


This looks like it will mis-warn in the case that someone doesn't provide neither default types nor type checker -- that doesn't seem like it's a deprecated case, is it?

I believe so? If neither are provided, we must use the old default behaviour, which relies on default_types? Those that directly use DraftXValidator will not see this as these provide a type checker.

We have to preserve the old behavior, but presumably in a new way -- the old behavior was "if you provide neither thing, you get a validator with type checking via some 'sane' defaults" -- the new way to do that would be to construct a suitably equivalent TypeChecker.

But I actually see that the code here is making the "new" default behavior be an empty type checker (with no types registered), but I can't see when that would actually be encountered, which is what's confusing me.

We have to preserve the old behavior, but presumably in a new way -- the old behavior was "if you provide neither thing, you get a validator with type checking via some 'sane' defaults" -- the new way to do that would be to construct a suitably equivalent TypeChecker.

Agreed.

But I actually see that the code here is making the "new" default behavior be an empty type checker (with no types registered), but I can't see when that would actually be encountered, which is what's confusing me.

I think maybe the confusion surrounds default behaviour and default values. The type checking behaviour always depends on the value of both default_types and type_checker.

The default value for type_checker is indeed an empty type checker. The default value for default_types is as it used to be, i.e. a sane type mapping. So to maintain the old behaviour - when neither are given, we redefine the empty type checker using the default type mappings.

Does that make sense? We cannot make the default for type_checker anything other than empty because we need to correctly process non-default-value default_types.

Is a user who does the following doing something wrong:

⊙ ~[jsonschema:python] -c 'import jsonschema; print jsonschema.validators.create({})' Julian@Macnetic jsonschema/validators.py:173: DeprecationWarning: default_types is deprecated, use type_checker DeprecationWarning, <class 'jsonschema.validators.Validator'>

they didn't provide default_types at all, why do they see a warning about it?

My thought process was that they are using it, just implicitly. However, I think you're right and this should not raise a warning. Any future removal of default_types should then preserve the same sane type checking defaults through the default value of type_checker.

Julian · 2017-12-24T15:52:55Z

jsonschema/validators.py

-            self._types.update(types)
+            self, schema, types=(), resolver=None, format_checker=None):
+
+            if types:


This won't warn in the (unlikely but possible) case that someone provides a falsy value to types, even though that code will break when the argument is removed.

Good catch. It should check for anything other than an empty tuple.

Julian · 2017-12-24T16:01:18Z

Just accidentally merged this in the process of trying to get Travis to behave :D -- still have a few things that need resolving but this is super close -- going to leave the comments here as I find them during review but totally alright with knocking off these myself as well if need be.

Julian · 2017-12-24T16:01:40Z

And thanks again! Now back to getting the rest of the stack done :)

Julian · 2017-12-27T13:06:31Z

docs/validate.rst

+        pass
+
+    def is_my_int(checker, instance):
+        return (Draft3Validator.TYPE_CHECKER.is_type(instance, "number") or


Ech, so I didn't know whether this was intentional or not, but now I see that there's some infinite recursion nonsense here when you upcall...

Is there? In order to use the is_my_int, a user must generate a new TypeChecker instance (via, e.g. redefine), which is then given to a new validator (via create or extend). In other words:

Draft3Validator.TYPE_CHECKER.is_type(instance, "number")

Is not changed.

(This was a half-baked comment that I just wrote to make sure I didn't forget to address it, sorry for the missing context -- it's the following:)

Not in the way you wrote it, but in the way I thought you mean to write it, which was:

return checker.is_type(instance, "number") or isinstance(instance, MyInteger)

That will do nasty infinite things because it calls itself, but I think the expectation there is clearly to upcall to the original definition of number.

And the above case was basically my motivation for suggesting we include the checker as one of the two arguments to the function (because it allows for forward extension).

The infinite recursion case though makes that a bit more complicated, it means there's a different thing you do if you're overriding the same type and want to upcall as if you're trying to access another type checker.

I think this needs a bit more thought -- my first reaction when I realized the above was "maybe we should pass in a third argument too, the original function from the checker that was redefined", but I think we just need to think through the 2 use cases again and make sure this API makes sense (the use case where you're accessing another checker, and the one where you're trying to upcall to the original one).

And the above case was basically my motivation for suggesting we include the checker as one of the two arguments to the function (because it allows for forward extension)

Apologies as I think we could have had this conversation earlier. I was concerned about attempted infinite recursion too but liked the ability to reference another type via checker.

The third argument doesn't immediately feel right to me - mostly because of the awkward basecase of a null function (in fact its not a null function, it's one that would raise an exception for an unknown type).

Will have a think about it.

Sorry for going AWOL on this - turns out changing jobs and moving takes up a fair bit of time.

As far as I remember, this point was the main remaining issue. I think I am in favour of the current API as a balance of simplicity and power. To me, the biggest benefit for passing in the type checker object (i.e. the current API) is for the ease of custom types, e.g. "string_or_int". In addition, it feels OK to be pretty explicit about which type checking function you are upcalling to in the case of a type redefinition.

Passing in both the object and function that is being overwritten feels like we are providing a somewhat bizarre OO inheritance mechanism, which I dislike.

What do you think? Perhaps we simply add a warning to the documentation on the potential for infinite recursion? If you're happy with that I'll sort a PR for this and the other minor comments.

Definitely no need to apologise, trust me staying at the same job and not moving is also pretty killer :(

I think the warning might be the thing, but I wanna reread my own comment again and see if a months time has given me any subconscious enlightenment (I doubt it...)

817b724 add perl implementation and test suite to the user list ca14e01 Merge branch 'pull/382' 3dabf55 move non-format tests out of the format directory, while keeping all ECMA regex tests together 4121aa5 move format tests to their own directory 4bae8aa Add more idn-hostname tests to draft-2019-09 and draft-07 6d91158 [325] Add some more hostname tests e593057 Merge pull request #389 from ssilverman/readme-drafts fb3766d README: Improve issue/PR links 79bef22 README: Update language around drafts ade47e4 README: Add Snow to the list of supporting Java validators fc0c14e README: Update simple JSON example 1167669 README: Update structure, consistency, spacing, and wrapping 9514122 Merge pull request #388 from json-schema-org/ether/maxProperties=0 7646490 test that maxProperties = 0 means the object is empty c3f4319 Merge pull request #384 from ChALkeR/chalker/unique 7766f48 Improve uniqueItems validation 7555d41 Add unnormalized $id tests 11f70eb [300] Add tests for valid use of empty fragments in "$id" b106ff0 [299] Add tests for invalid use of fragments in "$id" 4a74d45 Fix "tilde" spelling 3eca41b Merge pull request #379 from json-schema-org/ether/remove-wrapped-refs d61bae8 remove wrapped refs 536ec07 [359] Add unevaluatedProperties/unevaluatedItems cousin tests ac63eb7 Small README update that introduces the concept of directories 697944e Merge pull request #374 from karenetheridge/ether/allOf-anyOf-oneOf 33f8549 test all the *Of keywords together 44b99ed Merge pull request #373 from karenetheridge/ether/items-and-contains 4a2b52f some tests of items + contains 7f00cc8 add test that was present for other drafts but not 2019-09 a3f9e2e Merge pull request #371 from karenetheridge/ether/if-then-else-boolean aeeaecc some tests with if/then/else and boolean schemas b8a083c Merge pull request #372 from nomnoms12/unique-false-and-zero 85728f1 Add tests for uniqueness [1] and [true] fd01a60 Add tests for uniqueness [1] and [true] 0a8823c Merge pull request #370 from karenetheridge/ether/nul-char fa6f4dd add tests for the NUL character in strings 8bf2f7f Merge pull request #369 from ssilverman/data-desc 2ba7a76 Add data description 4f66078 Merge pull request #367 from karenetheridge/ether/additionalItems 283da7c some more tests for additionalItems 7ba95f3 add tests for keywords maxContains, minContains 2f2e7cf Merge pull request #365 from karenetheridge/ether/move-ecma-regex 8388f27 move ECMA regex tests under format/ git-subtree-dir: json git-subtree-split: 817b724b7a64d7c18a8232aa32b5f1cc1d6dd153

bsmithers added 2 commits November 27, 2017 10:09

Julian reviewed Nov 29, 2017

View reviewed changes

bsmithers added 4 commits November 29, 2017 19:56

Allow _type_checkers to be def'd as a dict at init

9c1c265

Converted to a pmap in attr's post init.

Revert "Allow _type_checkers to be def'd as a dict at init"

9467cbf

This reverts commit 9c1c265.

Use attr.ib's convert arg to generate the pmap

34227e8

- Cleaned than the post_init approach reverted in the previous commit

bsmithers added 3 commits December 11, 2017 13:48

Add checker instance to type checking functions

30bec1a

Improves composability, esp for custom types.

Minor changes addressing PR review issues

c202c48

Merge remote-tracking branch 'upstream/master' into types_draft4

f7ab18e

Conflicts: jsonschema/validators.py A change to the way bools are checked which appears to be redundant in the new type checker interface.

Simplify redefine_many.

e2264e5

Julian merged commit e2264e5 into python-jsonschema:master Dec 24, 2017

Julian reviewed Dec 24, 2017

View reviewed changes

Julian reviewed Dec 27, 2017

View reviewed changes

Julian mentioned this pull request Jan 25, 2018

Support namedtuple #364

Closed

Julian mentioned this pull request Aug 15, 2018

Fix infinite recursion in TypeChecker example #452

Merged


		type_checkers (pyrsistent.pmap):

		It is recommend to set type checkers through


		Arguments:

		instance (any primitive type, i.e. str, number, bool):

Widened type interface #374

Widened type interface #374

Conversation

bsmithers commented Nov 27, 2017

Julian commented Nov 29, 2017

Julian Nov 29, 2017 • edited Loading

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

Julian Nov 29, 2017 • edited Loading

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

Julian Dec 10, 2017 • edited Loading

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

bsmithers Nov 29, 2017 • edited Loading

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

Julian commented Nov 29, 2017

bsmithers commented Nov 29, 2017

Julian commented Dec 10, 2017

bsmithers commented Dec 10, 2017

Julian commented Dec 10, 2017

bsmithers commented Dec 11, 2017

Julian commented Dec 11, 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

Julian commented Dec 24, 2017

Julian commented Dec 24, 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

Julian Nov 29, 2017 •

edited

Loading

Julian Nov 29, 2017 •

edited

Loading

Julian Dec 10, 2017 •

edited

Loading

bsmithers Nov 29, 2017 •

edited

Loading