rename target arg so we can use targets requiring an argument 'target' #1106

dchudz · 2018-02-12T03:37:20Z

This is my attempt to address #1104, following the suggestion in the comment from @DRMacIver on that issue.

But I don't think it's good:

calling builds() with no argument for the target no longer raises TypeError (builds().example() does), since the new code here isn't actually run when you call builds()
for the same reason, the deprecation warning doesn't show when you first call builds()

I'm not thinking of any ways to fix this without adding a bunch of complication (I'd have to mess with proxies() and/or base_defines_strategy()?).

I'm completely new here, but it seems to me that merging this would make life worse for users and developers.

I figured I'd post it anyway, since (a) maybe I'm wrong about that or (b) I'd love to be pointed in a better direction if anyone has time to set me right.

Or if not, we can just close this and add to the issue: "Don't fix it THAT way!"

dchudz · 2018-02-12T05:03:31Z

Also:

I'll add a RELEASE.rst if this is, or turns into, something we actually want to do.
The test failures seem to be about the __repr__. Can work that out if this is something we actually want to do.
I've checked my employment agreement, and I'll own the my contributions, so they can be licensed appropriately.

Zac-HD · 2018-02-12T06:00:15Z

Hi @dchudz - thanks for contributing 🎉 We're always delighted to help someone with a PR, be it the first or hundredth!

We generally think of 'lazy errors' as a feature rather than a bug, because it means that the problem is reported when a test is executed rather than when a module is imported. Admittedly that's not useful here, but it's also not a blocker.
Great to hear legal is clear and that you've seen the release note process. Generally I write one early so that the build turns green as soon as possible in the proof-of-concept stage, but that's a personal thing. Also - don't forget to add yourself to the list of contributors!
How did you find the docs for a first contribution? What could we improve beyond merging Add a guide for working on internals #1100? It's hard for us to see them as first-timers, so any feedback you have would be appreciated 😄

The __repr__ bit is trickier, but solvable - I'd use a slightly different solution:

def builds(*target_and_args, **kwargs):
    if target_and_args:
        target, *args = target_and_args
    elif 'target' in kwargs:
        note_deprecation(...)  # deprecated, give target as the first posarg, etc
        target = kwargs.pop('target')
    else:
       raise InvalidArgument(...)  # target must be given as the first posarg, etc
    ...

This avoids adding a new keyword argument, which was changing the repr - and also making the problem less common instead of fixing it ("I can't build things with a keyword only arg named ..."). The downside is that this makes the signature a bit less useful, but IMO it's a net improvement if we also use a descriptive name. Possible that @DRMacIver will veto this API though, so if your time is at a premium you might want to let him chime in before implementing.

dchudz · 2018-02-12T06:39:34Z

I'd use a slightly different solution

Yeah, I was tempted by this, but pushed away by the less useful signature and @DRMacIver's example code in the issue doing it this way. (That said, the argument name hypothesis_internal_target doesn't make the signature super pretty either!)

I like your approach - I'll do that tomorrow night if I haven't seen a veto here first.

How did you find the docs for a first contribution?

The only frustrating thing was having trouble running just a single unit test (b/c of the test-specific dependencies). (I eventually deleted tox.ini locally and then used pytest normally, before I saw #1100.) Merging #1100 will help, but I'd still suggest one or more of:

using tests_require= in setup.py
something like development_requirements.txt.
mentioning what's needed in testing-hypothesis.rst (I'd probably look there before I looked in How to Work on Hypothesis Internals)

How to Work on Hypothesis Internals seems like it would be a good place for your point about preferring 'lazy errors'. (I agree with avoiding import errors, but I don't think I understand yet why a user error would ever lead to problems at import time. But I haven't seen much of Hypothesis yet.)

The docs were fun to read. :) (Reading them with my morning coffee was how I got interested enough to decide to see if I could find an issue easy enough for me.)

I spent a little while being confused by base_defines_strategy since most of my experience with decorators has the decorated function call the undecorated one, rather than the more subtle thing going on here where keep it for later. (A docstring on base_defines_strategy probably would have helped me, but just having to work it out seems okay too.)

dchudz · 2018-02-12T06:53:52Z

Oh, I guess you mean you don't want errors when a user's module is imported. (For some reason I thought you meant the import of hypothesis itself.)

I guess I'm still a bit confused by that, though. If you know I wrote broken test code even before you run any tests, I feel like I'd prefer to be told then. But I realize I'm missing tons of context, and of course it's fine if you don't have time to help me get it. :)

Zac-HD · 2018-02-12T08:01:20Z

There's going to be a note about test dependencies, and requirements/test.{in,txt} already exist - but I'll add the tests_require key 😄

The preference for errors at runtime instead of import time is purely because it allows other tests to run, and therefore pass or fail for other reasons - useful if you're eg. selecting only unrelated tests to run. We want Hypothesis to be all correct all the time, but not everyone shares that view and better a failing test than no test! This is definitely a subjective decision that could reasonably go the other way, FWIW.

The docs were fun to read. 😄 (Reading them with my morning coffee was how I got interested enough to decide to see if I could find an issue easy enough for me.)

😍 Docs so good that they actually attract contributions?! 😍
(Oh, follow-up - how did you pick an issue? Were labels helpful, titles, something else... ?)

base_defines_strategy (and the nested functions) totally should have docstrings - that would make it much easier to follow. I suggest, in order of increasing nesting:

Returns a decorator for strategy functions. If force_reusable is True, the generated values are assumed to be reusable, ie immutable and safe to cache, across multiple test invocations.
(hey, @DRMacIver - shouldn't none() be decorated with defines_strategy_with_reusable_values? - it's not)
A decorator that registers the function as a strategy and makes it lazily evaluated.
accept doesn't need a docstring IMO

DRMacIver · 2018-02-12T11:49:59Z

Thanks for this seconded @dchudz!

Ugh, sorry about the repr issue. That's an annoying thing that I didn't think of when I proposed my original solution. My bad. I think @Zac-HD's suggestion of putting it in *args is the simplest way forward without opening giant cans of worms, so my recommendation would be to go down that route.

That being said, I don't think the suggested code is quite right. As builds can take positional arguments as well as kwargs, this will do the wrong thing if you have something like builds(integers(), target=f). I think the thing to do here is to check if len(args) > 1 and not isinstance(args[0], SearchStrategy) and use args[0] as target if that returns True. The only valid types in arguments to builds are a single function for the target and everything else is a Strategy, and these can't overlap so using type checking here is legit I think.

Agreed this makes the signature a bit ugly, but I don't think we have a way out of that right now. Long term I think the right thing to do is to get a bit more flexibility in how we display those reprs - currently it treats it as if it's always more sensible to show arguments in their keyword form. This is so that something like e.g. integers(0, 10) expands as the clearer integers(min_value=0, max_value=10), but as this example shows there are definitely cases where that's not the right approach! I definitely don't recommend diving into that code in your first PR though, as it's a bit unpleasant.

I guess I'm still a bit confused by that, though. If you know I wrote broken test code even before you run any tests, I feel like I'd prefer to be told then. But I realize I'm missing tons of context, and of course it's fine if you don't have time to help me get it.

There are a couple of reasons we prefer to only throw errors at test execution time. The big ones are:

Errors at test import time tend to throw people and be correspondingly hard for them to debug. There's an expectation that errors in your test code result in failures in your tests, and the fact that that test code happens to be defined in a decorator doesn't seem to change that expectation for people.
Things like deprecation warnings etc. localize better when they happen inside the test - test runners will often swallow them or put them in silly places if they're at import time, but will attach any output that happens in the test to the test itself.
There are a lot of cases where raising an error, deprecation warning, etc. is only possible in a test - e.g. if you're using the inline style with data, or if you're using flatmap or composite then the strategy won't actually get evaluated until we run the test, so that's the only place they can happen. It's nice to be consistent, and it's weird if sometimes strategy errors result in definition time errors and sometimes they result in test errors.

(This should probably go in the guides and maybe public documentation somewhere, as I agree this is non-intuitive).

DRMacIver · 2018-02-12T11:52:44Z

Additional questions and answers:

using tests_require= in setup.py

Does tests_require actually do anything for anything other than setup.py test (which we don't support)?

(hey, @DRMacIver - shouldn't none() be decorated with defines_strategy_with_reusable_values? - it's not)

I think it might be OK not to given that it calls just which does, but it would probably be clearer if it did yeah.

base_defines_strategy (and the nested functions) totally should have docstrings

Agreed. I don't think it makes sense to add docstrings to nested functions (especially ones that will inherit them from the wrapped function!) but the actual decorator should definitely have a docstring.

Zac-HD · 2018-02-12T11:57:20Z

Builds may accept any callable, not just a function (eg a class); otherwise well spotted - I (obviously) missed that bug 😥

(This should probably go in the guides and maybe public documentation somewhere, as I agree this is non-intuitive).

#1100 again?

In this test_requires is basically documentation, but I think there are some environment managers that pick it up too.

@dchudz if you're adding docstrings, you could also switch over the decorator on none() 😄

DRMacIver · 2018-02-12T12:17:12Z

#1100 again?

I'm tempted to object on grounds of scope creep, but decided it would be easier to just do it. 😉 This is the last addition though!

DRMacIver · 2018-02-12T12:26:59Z

Builds may accept any callable, not just a function (eg a class)

Whoops. True, yes. I think what I said still applies though - strategies that we define are never callable, and users can't define their on strategy subclasses, so there's still no overlap.

dchudz · 2018-02-12T14:52:40Z

Wow, thanks for all the answers/explanation! After work today I'll fix this up and also open a PR with @Zac-HD's suggested docstrings for base_defines_strategy + decorating none() with defines_strategy_with_reusable_values.

how did you pick an issue?

I kind of picked the first one in the list, but also the tags (help wanted, good first issue) were great.

requirements/test.{in,txt} already exist

Ugh, oops/sorry. Silly me. I even saw requirements/tools.txt, but not that.

Does tests_require actually do anything for anything other than setup.py test (which we don't support)?

I don't think so, my mistake. I really meant to suggest tests_require= with a dev or test section like this. Then we can do pip install -e .[dev]. E.g. replacing the pip install . in guides/internals.rst (after #1100) with this would mean you can skip the following two pip installs.

dchudz · 2018-02-12T14:55:57Z

if you have something like builds(integers(), target=f)

@DRMacIver Note that builds's current API doesn't support this (got multiple values for keyword argument 'target'), so this could be an InvalidArgument if we want. (Still requires a check like the one you suggested.) I'm happy to do whatever you want, but I think the InvalidArgument is my preference (since it seems silly to add new behavior that's already deprecated).

dchudz · 2018-02-12T14:58:33Z

Despite the ugly signature, def builds(*target_and_args, **kwargs): is pretty nice - really the right API for this function is to insist on the target as a (the first) positional argument, and Python doesn't give us any other way to do that.

Zac-HD · 2018-02-12T23:11:39Z

I'd just stick the docstrings and decorators in this pull, albeit a separate commit - drive-by refactoring is OK when you're in the area.
pip install -e .[dev] it is 😄
If it's not currently supported, let's keep it as an InvalidArgument - adding pre-deprecated behaviour is indeed silly!

dchudz · 2018-02-13T00:28:04Z

tests/cover/test_direct_strategies.py

+        ds.builds().example()
+
+
+@pytest.mark.parametrize('non_callable', [1, 'abc', ds.integers()])


I guess this could be a hypothesis-style randomized test? But writing a strategy for "non-callable" didn't really seem worth it.

Parametrize is fine here - Hypothesis is generally better at "works for all valid inputs" than "fails for all invalid inputs"; the latter space is enormous and mostly trivial anyway.

Zac-HD

Nice work! This is looking good, and I really like the tests 😀

I've left fairly detailed comments about style, and one substantive change to the error you'd get if no posarg is provided. Fix those, and I'll be delighted to merge it in 🎉

Zac-HD · 2018-02-13T01:21:17Z

RELEASE.rst

@@ -0,0 +1,3 @@
+RELEASE_TYPE: patch
+
+This release fixes :func:`builds(callable) <hypothesis.strategies.builds>` so that ``target`` can be used as a keyword argument for passing values to the target. The target itself can still be specified as a keyword argument, but that behavior is now deprecated. The target should be provided as the first positional argument.


Our convention for strategies is to use the default link text for all but decorators, so just

:func:`~hypothesis.strategies.builds`

Wrap lines at 79 characters - the old convention is still useful for diffs etc., and we enforce it for the code (see the lint job on Travis).

Zac-HD · 2018-02-13T01:22:17Z

src/hypothesis/strategies.py

@@ -945,11 +955,26 @@ def builds(target, *args, **kwargs):
    the target.

    """
+    if target_and_args:
+        target, args = target_and_args[0], target_and_args[1:]


~~I would write this target, *args = target_and_args - same effect and considerably shorter.~~

Sadly, doesn't work in Python2. Maybe in 2020? :)

Can't come soon enough 😭

Zac-HD · 2018-02-13T01:23:13Z

src/hypothesis/strategies.py

+    if target_and_args:
+        target, args = target_and_args[0], target_and_args[1:]
+        if not callable(target):
+            raise InvalidArgument('The first positional argument to builds() must be a callable'


Needs a space at the end of this string, to avoid joining words when it's concatenated.

Zac-HD · 2018-02-13T01:24:02Z

src/hypothesis/strategies.py

    if infer in args:
        # Avoid an implementation nightmare juggling tuples and worse things
        raise InvalidArgument('infer was passed as a positional argument to '
                              'builds(), but is only allowed as a keyword arg')
-    hints = get_type_hints(target.__init__ if isclass(target) else target)
+    hints = get_type_hints(target.__init__


No need to change this line.

oh right, sorry, that was for line length in the older version that introduced the longer name hypothesis_internal_target

Zac-HD · 2018-02-13T01:28:01Z

src/hypothesis/strategies.py

+        if not callable(target):
+            raise InvalidArgument('The first positional argument to builds() must be a callable'
+                                  'target to construct.')
+    elif 'target' in kwargs:


elif 'target' in kwargs and callable(kwargs['target']):

Handles the case where someone passed target=some_strategy but did not supply any positional arguments - we want that to be the generic "no callable supplied" error, and weren't checking.

Good point, I'll also add a test that would have failed w/o this fix.

Zac-HD · 2018-02-13T01:30:55Z

src/hypothesis/strategies.py

+                         'Provide it as the first positional argument instead.')
+        target = kwargs.pop('target')
+    else:
+        raise InvalidArgument('No target was provided to builds().'


No space at end of string, and I'd prefer to avoid calling the callable "target". How about: "builds() must be passed a callable as the first positional argument, but no positional argument was given."

As a general tip, for longer strings we generally start them on the line after the opening parenthesis, indented by four spaces. There are some examples in eg. decimals().

I'd prefer to avoid calling the callable "target".

Makes sense. Unless you object, I'm still referring to 'target' in the deprecation message ("Specifying the target as a keyword argument to builds() is deprecated ...") since in that case the user just called it target so that naming is familiar to them.

Yep, I agree that it should be kept in the deprecation message.

I'll clean up some other references to "target" I accidentally left in too.

Zac-HD · 2018-02-13T01:31:43Z

tests/cover/test_direct_strategies.py

@@ -225,6 +227,29 @@ def test_produces_valid_examples_from_args(fn, args):
    fn(*args).example()


+def test_build_class_with_target_kwarg():
+    NamedTupleWithTargetField = collections.namedtuple('Something', ['target'])
+    ds.builds(NamedTupleWithTargetField, target=ds.integers())


Need to add a .example() on the end, or there could be a lazy error lurking 😄

Zac-HD · 2018-02-13T01:33:32Z

tests/cover/test_direct_strategies.py

+        ds.builds().example()
+
+
+@pytest.mark.parametrize('non_callable', [1, 'abc', ds.integers()])


Parametrize is fine here - Hypothesis is generally better at "works for all valid inputs" than "fails for all invalid inputs"; the latter space is enormous and mostly trivial anyway.

dchudz · 2018-02-13T02:33:48Z

More detailed summary of API changes in case this helps anyone:

target can now be used as a keyword argument for values that will be passed to the target
specifying the target as a keyword argument to builds() now issues a deprecation warning
various bad ways of calling builds() differ in:
- when the exception is raised - these situations raise exceptions in the first few lines of the inner wrapped builds() function (in some cases this is later than previously, and in one case -- non-callable 1st positional arg -- it's earlier)
- which exception is raised -- now they raise InvalidArgument ~~instead of TypeError (I'm following @Zac-HD's example and InvalidArgument does seem more natural for something we're raising ourselves)~~ (which inherits from TypeError, the old exception)

dchudz · 2018-02-13T02:42:01Z

Thanks for the great comments, @Zac-HD!

Zac-HD

FYI InvalidArgument inherits from TypeError, so except TypeError or with pytest.raises(TypeError) will still catch it - as will trying to handle the top-level HypothesisException 😄

Thanks for the great comments

😊. Thanks for the great PR!

Final (I think) round of comments: some trailing whitespace, and a last rename:

Zac-HD · 2018-02-13T02:40:56Z

src/hypothesis/strategies.py

@@ -928,23 +937,41 @@ def do_draw(self, data):

 @cacheable
 @defines_strategy
-def builds(target, *args, **kwargs):
+def builds(*target_and_args, **kwargs):


It now occurs to me that we should probably name this argument *callable_and_args, to banish the word "target" entirely. Sorry 😞

Oh yeah, makes sense.

Zac-HD · 2018-02-13T02:44:32Z

RELEASE.rst

@@ -0,0 +1,6 @@
+RELEASE_TYPE: patch
+
+This release fixes :func:`~hypothesis.strategies.builds` so that ``target`` 


Remove trailing whitespace here. (thanks, linter)

dchudz · 2018-02-13T02:53:55Z

FYI InvalidArgument inherits from TypeError, so except TypeError or with pytest.raises(TypeError) will still catch it.

oh right, thanks!

Zac-HD · 2018-02-13T03:00:06Z

This looks good to me, but I'll give @DRMacIver a chance to review it instead of merging (and releasing!) right away.

In particular, he might want a minor rather than a patch release (I'm happy with a patch) - on one hand it's a bugfix and backwards-compatible; on the other there is a slight change to when errors are raised.

DRMacIver · 2018-02-13T07:54:08Z

In particular, he might want a minor rather than a patch release (I'm happy with a patch)

I actually hadn't thought of it, but now that you mention it I actually think I do! I've added a ticket to pin down what the actual rules are around this (#1109), but in particular I think we shouldn't change the signature of public facing APIs in patch releases even in this very compatible way.

DRMacIver · 2018-02-13T07:55:13Z

@DRMacIver Note that builds's current API doesn't support this (got multiple values for keyword argument 'target'), so this could be an InvalidArgument if we want. (Still requires a check like the one you suggested.) I'm happy to do whatever you want, but I think the InvalidArgument is my preference (since it seems silly to add new behavior that's already deprecated).

Whoops. Good point. Totally agreed with your call.

DRMacIver

Added some comments inline, but generally LGTM. 🎉 Thanks for the patch! Once the minor/patch thing is addressed (and the wording comment I added if you care), let us know you're done and one of us will happily merge!

DRMacIver · 2018-02-13T07:56:10Z

src/hypothesis/strategies.py

+    else:
+        raise InvalidArgument(
+            'builds() must be passed a callable as the first positional '
+            'argument, but no positional argument was given.')


Super minor nit: "no positional arguments were given".

DRMacIver · 2018-02-13T07:56:59Z

RELEASE.rst

@@ -0,0 +1,6 @@
+RELEASE_TYPE: patch


As per discussion in comments, I think this needs to be a minor release rather than a patch.

dchudz · 2018-02-13T15:36:23Z

Hmm, the failing job says The job exceeded the maximum time limit for jobs, and has been terminated.

Seems unlikely this is really related to my PR. Help...?

DRMacIver · 2018-02-13T15:39:44Z

Help...?

Yeah, sorry, Travis can get a bit flaky when there are as many build jobs as we have and it's otherwise under load! Sorry about that. I've restarted the failing job and expect it will go green, as it's unlikely in the extreme that this is anything you did. 😄 Will click merge once it's done!

DRMacIver · 2018-02-13T16:00:33Z

🎉

dchudz · 2018-02-13T16:09:12Z

Yay!

DRMacIver · 2018-02-13T20:39:48Z

Sorry our amazing auto-deployment system is being less than amazing due to Travis flakiness. :-( I've tried restarting the failing bits of the build once already and it failed again. Trying once more... Should hopefully ship it soon!

Zac-HD · 2018-02-13T23:56:58Z

And, it's released! Thanks again @dchudz and congratulations 🎉✨

rename target arg so we can use targets requiring an argument 'target'

e2f21f2

Zac-HD mentioned this pull request Feb 12, 2018

3.44.24 breaks AppVeyor Python 3.4 builds #1091

Closed

dchudz added 9 commits February 12, 2018 19:02

add myself to contributors

5344b5b

write release note

15f2579

specify target and args for build() together as *target_and_args

0d4cba7

add docstrings in base_defines_strategy

d073aa4

don't try to assign to *args

347fea9

don't use backticks in exception messages

ed7da80

don't refer to hypothesis_internal_target from bad implementation

d170e00

remove old implementation

f27567c

remove old implementation

124252f

dchudz commented Feb 13, 2018

View reviewed changes

fix formatting

25df8a3

Zac-HD requested changes Feb 13, 2018

View reviewed changes

fix based on Zac's code review

f02799c

dchudz changed the title ~~[work in progress] rename target arg so we can use targets requiring an argument 'target'~~ rename target arg so we can use targets requiring an argument 'target' Feb 13, 2018

Zac-HD reviewed Feb 13, 2018

View reviewed changes

dont use 'target' in naming arg to builds()

9d97478

Zac-HD approved these changes Feb 13, 2018

View reviewed changes

DRMacIver mentioned this pull request Feb 13, 2018

Document necessary and sufficient conditions for version number bumps #1109

Closed

DRMacIver approved these changes Feb 13, 2018

View reviewed changes

address more code review comments

19b0735

DRMacIver merged commit f6eb0e8 into HypothesisWorks:master Feb 13, 2018

dchudz mentioned this pull request Feb 14, 2018

strategy.builds can't create a class with a keyword argument named "target". #1104

Closed

Zac-HD mentioned this pull request Mar 11, 2020

Parameter name clash in extra.django.from_model #2369

Closed

		ds.builds().example()


		@pytest.mark.parametrize('non_callable', [1, 'abc', ds.integers()])

		@@ -0,0 +1,3 @@
		RELEASE_TYPE: patch

		This release fixes :func:`builds(callable) <hypothesis.strategies.builds>` so that ``target`` can be used as a keyword argument for passing values to the target. The target itself can still be specified as a keyword argument, but that behavior is now deprecated. The target should be provided as the first positional argument.

		@@ -0,0 +1,6 @@
		RELEASE_TYPE: patch

		This release fixes :func:`~hypothesis.strategies.builds` so that ``target``

rename target arg so we can use targets requiring an argument 'target' #1106

rename target arg so we can use targets requiring an argument 'target' #1106

Conversation

dchudz commented Feb 12, 2018 • edited

dchudz commented Feb 12, 2018 • edited

Zac-HD commented Feb 12, 2018

dchudz commented Feb 12, 2018

dchudz commented Feb 12, 2018

Zac-HD commented Feb 12, 2018 • edited

DRMacIver commented Feb 12, 2018

DRMacIver commented Feb 12, 2018 • edited

Zac-HD commented Feb 12, 2018

DRMacIver commented Feb 12, 2018

DRMacIver commented Feb 12, 2018

dchudz commented Feb 12, 2018

dchudz commented Feb 12, 2018

dchudz commented Feb 12, 2018

Zac-HD commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD Feb 13, 2018 • edited

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

dchudz commented Feb 13, 2018 • edited

dchudz commented Feb 13, 2018

Zac-HD left a comment • edited

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

dchudz commented Feb 13, 2018

Zac-HD commented Feb 13, 2018

DRMacIver commented Feb 13, 2018

DRMacIver commented Feb 13, 2018

DRMacIver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchudz commented Feb 13, 2018 • edited

DRMacIver commented Feb 13, 2018

DRMacIver commented Feb 13, 2018

dchudz commented Feb 13, 2018

DRMacIver commented Feb 13, 2018

Zac-HD commented Feb 13, 2018

dchudz commented Feb 12, 2018 •

edited

dchudz commented Feb 12, 2018 •

edited

Zac-HD commented Feb 12, 2018 •

edited

DRMacIver commented Feb 12, 2018 •

edited

Zac-HD Feb 13, 2018 •

edited

dchudz commented Feb 13, 2018 •

edited

Zac-HD left a comment •

edited

dchudz commented Feb 13, 2018 •

edited