centralize errors #364

shmax · 2017-02-26T03:35:14Z

Some improvements to the error system in an effort to make things a little easier for people interested in adding localization, such as #363:

Added a new class: ConstraintError. This extends an enum class which I've brought into the project to ensure that nobody types constraint types or messages directly into the code (by typehinting addError). I've used it before in a number of projects and have found it quite useful.
Converted all error strings into a format that can be passed to sprintf.
I had to invent a few new format error types to avoid ambiguity: phoneFormat, ipFormat, styleFormat, timeFormat, urlFormat.

@bighappyface @erayd @harej

erayd · 2017-02-26T06:39:18Z

@shmax Thanks for tackling this :-). I'm currently sick with some kind of nasty bug and can't think straight, but will try to review this properly in the next couple of days.

In the meantime, quick thoughts after glancing at it:

Why are you depending on an unstable branch of php-enum? Is there a reason you can't use a stable release?
Need to run composer style-fix; the travis builds are currently failing with code style errors.
If you're changing the API anyway, then could $path be bundled in with ConstraintError? If we're making a new class for this, might as well make sure it has all the information.
Same point as above re additional format-specific error information.
I like the syntax for selecting an error to use - assume php-enum is using __callStatic()? Good way of doing it.
Does php-enum allow extending the class and only defining a subset of new errors, and falling back to the parent definitions for missing stuff? That's really important - need it to avoid breaking people's translations whenever a new error is added. Your current approach requires the user to manage falling back to parent::getMessage() manually in some way, which seems like asking for trouble later when somebody doesn't think to do it.

[Ed: One other thought - any chance of more descriptive commit messages than 'ws'? That doesn't mean anything to me; have to click through to the commit to figure out what it is supposed to be.]

shmax · 2017-02-26T06:46:03Z

I'll squash in a bit... I couldn't get the style checker to work locally, so I've been letting travis figure it out. Consider it a wip for now. I'm out and about at the moment, but will resume in the morning.

erayd · 2017-02-26T06:47:12Z

Gotcha. What's the style-checker doing? It should run on everything but nightly / hhvm; if it's not working on your system then something rather strange is going on.

erayd · 2017-02-26T06:48:26Z

You can also get an executable phar here, which means you can run it without needing to go through composer - if you're having issues with the depended one, this may be a suitable temporary workaround.

shmax · 2017-02-26T10:13:38Z

It's something to do with line endings. I'm on Windows, and I've got git configured to sort them out when pushing and pulling, but apparently the style checker isn't as savvy--it just aborts right away, and doesn't get as far as doing any style analysis.

shmax · 2017-02-26T10:59:10Z

Why are you depending on an unstable branch of php-enum? Is there a reason you can't use a stable release?

No reason. Updated to latest release.

If you're changing the API anyway, then could $path be bundled in with ConstraintError? If we're making a new class for this, might as well make sure it has all the information.

Not sure what you're suggesting. $path is dynamic.

Same point as above re additional format-specific error information.

Not following.

That's really important - need it to avoid breaking people's translations whenever a new error is added.

I hadn't really gotten as far as deciding how to advise people wanting to localize. We could provide extra hooks, but really they could just do as @bighappyface suggests and post-process errors as they're caught. Definitely don't expect people to subclass ConstraintError, at least not in its current state. The only real difference is that now the meaning of $constraint is no longer ambiguous, and we have a clear, easy-to-read mapping of error strings to error codes in once place for reference.

As far as breaking their strings goes, well, I guess I don't follow that, either. I wouldn't expect adding new strings to break anything--they just wouldn't be localized until the client does something about them. We'll have to consider existing $constraint values to be part of the external API going forward, and be careful to update the release version accordingly if they ever do change.

shmax · 2017-02-26T17:54:33Z

I did realize that with the old error layout it would be difficult (though not impossible) for localizers to extract the arguments for their translation calls; everything was just mixed into the same error object. I've restructured constraint to be an array with keys for name and params, which should sidestep the difficulty. It will break bc for anyone that was doing anything programmatic with the old fields in the error arrays, unfortunately.

shmax · 2017-02-28T16:38:54Z

@bighappyface @erayd So what do you think?

erayd · 2017-02-28T16:43:09Z

@shmax I'm still sick, so I can't promise the world's best feedback, but I will take a decent look at this today and let you know.

shmax · 2017-02-28T19:15:04Z

No rush. What about you, @bighappyface? You seemed to have some opinions about this kind of thing...

bighappyface · 2017-02-28T22:09:59Z

While I am not opposed to the approach per se I am concerned by how loose we have become with this package and breaking changes.

Simply put, changing the major version of this package every third PR that comes in is frustrating to me and unfair to consumers. We have to do a better job at trying to make things better without redesigning them every time.

That being said, I ask that we start road-mapping releases and embrace the idea of breaking the API later while maintaining BC now. If we are to break the API then we should set goals for what needs to be changed and bundle them together as part of a plan and not a side-effect of any given PR. This will help us vet our changes and be far more prudent in determining what needs to happen, and how to get there, with due dilligence.

In this case that means not altering the method signature for addError nor it's return value array structure for the constraint element. Those changes can come as the second phase to this work, in another major release, where this first phase centralizes errors, establishes constants, and basically ends there.

bighappyface · 2017-02-28T22:13:14Z

To help on this effort I have linked this PR and the original issue to a new 5.2.0 project:

https://github.com/justinrainbow/json-schema/projects/1

shmax · 2017-02-28T22:40:43Z

I can see the wisdom in not altering the layout of the error object, but what's the harm in changing the addError signature? Do you consider that part of the public API?

erayd · 2017-02-28T22:40:54Z

@bighappyface I agree very strongly with everything you just said in those last two posts.

Not sure how much of the discussion around the v5 refactor you read (it started in the defaults PR and then moved to its own thing), but I was very much in favour of not breaking things, and decided to lose that fight, because I'm new here and the impression I was getting was that maintaining backwards compatibility was at best a minor concern, and you guys didn't care nearly as much about it as I wanted to - the understanding I took from those conversations was that, as far as this library is concerned, minimising the number of public methods on Validator was preferable to maintaining backwards compatibility. I don't regret the code that resulted (although I'm very pleased that the check / coerce aliases on Validator ended up back in the mix), or the valuable contributions others here (particularly @shmax) made to help bring it to that final state, but if maintaining backwards compatibility in the API has now become a priority, or my original understanding was incorrect, I am very much in favor of that.

To that end, would it be possible to also have as a project goal properly documenting the public API? I'm happy to assist with the parts I'm familiar with, but I don't know enough about the intent behind the various parts to do much of it justice. Currently, Validator seems to be the only 'official' public interface, but then vast swathes of the rest of the API seem to be considered unofficially part of what users are intended to interface with - there is no clear line here.

erayd · 2017-02-28T22:44:47Z

@shmax That's why I reckon the public API needs documenting (see above). It's not officially documented as part of the public API, but it seems to be treated like something an end user would interact with. Certainly getErrors is pretty obviously public by virtue of its intended use.

erayd · 2017-02-28T22:47:42Z

@shmax Noting @bighappyface's request, would you still like me to do a full code-review of this PR today, or would you prefer to make changes first?

shmax · 2017-02-28T22:53:06Z

Is there really that much to review? Here's the takeaway:

Moved error strings to ConstraintError, and did an accounting of all the possible values for $constraint (to eliminate ambiguity).
Changed the addError signature to take a ConstraintError (to enforce compliance with 1.)
Changed the format of the error object so that the constraint arguments can be easily extracted by someone wanting to pass them to their own localization mechanic.

That's pretty much it. @bighappyface disagrees with both 2. and 3., at least without some kind of long-term release plan, which is fine. I guess I'll just wait for further instruction from him.

erayd · 2017-02-28T23:06:46Z

@shmax When I do code reviews, I like to understand the full impact of every single changed line, and the full implications of any logic that changed as a result, whether intended or otherwise. This can take a while.

If all you're after is a review of the concept, then I'm very much in favour of tidying up the way errors are handled - I had a PR in the works myself following our discussion in the schema-validation PR, (although not yet at the point I'd be happy to submit it), however you beat me to it - you seem keen to tackle this task, and you doing it means I can keep focused on the schema validation (and that annoying recursion bug), so I'm all in favour of this one being yours :-).

Between my original review comment and what @bighappyface has said, there probably isn't too much more for me to say here unless you'd like a full code review, which I'm happy to do if you'd find it valuable, and was my original intention. If a code review isn't what you want, but you have specific questions or want feedback on something, please ask.

bighappyface · 2017-03-01T04:08:09Z

@shmax sorry to see you close and delete. I don't think you needed to go that far.

May I suggest a variation?

# before
$this->addError($path, 'There must be a minimum of ' . $schema->minItems . ' items in the array', 'minItems', array('minItems' => $schema->minItems));

# after
$this->addError($path, ConstraintError::MIN_ITEMS(), ConstraintError::MIN_ITEMS, array('minItems' => $schema->minItems));

This change would maintain the core of your contribution while also keeping the current method signature. I think we gain a great deal by centralizing and we can refactor addError in 6.0.0.

What do you think?

Regarding 5.0.0, you guys did a ton of work adding some necessary functionality that this package sorely needed to stay relevant. I felt your work merited a version, and your continued engagement and support merits much more. Now that a bit of a team is forming I think organizing will help us tremendously, and I hope to see this package continue to improve. Careful planning will focus work and help motivate efforts.

shmax · 2017-03-01T04:37:08Z

$this->addError($path, ConstraintError::MIN_ITEMS(), ConstraintError::MIN_ITEMS, array('minItems' => $schema->minItems));

That's still a different signature. The second argument to addError as it stands now is currently a string, whereas in your sample the second argument is an instance of ConstraintError (which extends Enum). I guess I don't see the rationale in considering something like addError to be an inviolable part of the public API--by that logic we should never touch, add, or remove any public methods on any of the constraint classes, and I think that might be taking things a bit too far.

I can go ahead and rearrange things a bit so that the original signature IS maintained (at least until 6.0) as you suggest, but we wouldn't have achieved the goal of helping the folks who want to localize without making the change to error output (we'd be closer, but they would be left in the position of having to write a lot of glue code to interpret each of the possible values for error.constraint).

Thanks for the encouragement. When do you see us doing a 6.0 release?

erayd · 2017-03-01T04:47:42Z

@bighappyface - Works for me :-).

@shmax - I agree that your work is valuable, and closing this PR isn't necessary - with a few tweaks it can be made to work while still maintaining backwards-compatibility. it doesn't change the signature because the current approach doesn't define a type, so we can accept any argument we like there. It doesn't have to be a string.

@everyone I do think we need to seriously consider how people will extend the error messages to their own subset though - we can certainly come up with a viable approach for that now, and without needing to change the existing method signatures. I don't think we need to actually create translations, but I do think we need to have a sensible approach that we can point to and tell users who want to translate to use, and it needs to be a strategy that allows people to only have a subset of the messages translated, and does not require them to catch errors. IMO that translation should not be making the user interpret the output; it makes much more sense to bake some kind of way to apply translations into the message resolver you're busy writing here. That could be as simple as passing it an array of translations, and just merging it into the active messages array.

Regarding which parts of the error API are considered public - this is why I think we really need some better documentation regarding what is public API, and what is not. There's no clear line right now, and it's extremely frustrating trying to work on this without knowing what's safe to step on without breaking things for users.

shmax · 2017-03-01T05:01:02Z

it doesn't change the signature because the current approach doesn't define a type, so we can accept any argument we like there. It doesn't have to be a string.

Okay, fine, it's true that the signature doesn't technically change, but if we're going on the assumption that people are currently making their own calls to addError--which I admit is possible--and passing in strings in good faith (or overriding the method and doing something of their own with the incoming string before passing it down to the base), then their code will still break.

As far as the specifics on localization go, let's not make any assumptions. There are many different schemes, and not all of them involve typing strings into the code (in fact I've never personally seen it done that way). More typically the translated strings come from a database, or generated resources that are maintained in a completely separate process. The changes I made in this PR did provide one crucial piece to help make them all possible, however, which was to separate the arguments that you pass along with the string from the rest of the information in the error array. As things stand now (in master), you would have to write a big crazy switch statement.

Anyway, I'll mull it over a bit more and see if I can come up with some kind of transitional compromise.

erayd · 2017-03-01T05:05:28Z

Okay, fine, it's true that the signature doesn't technically change, but if we're going on the assumption that people are currently making their own calls to addError--which I admit is possible--and passing in strings in good faith (or overriding the method and doing something of their own with the incoming string before passing it down to the base), then their code will still break.

It doesn't need to break if you add a simple typecheck to the method (not in the signature). If it's an instance of your constraint class, use the new behavior. Otherwise, stick with the old behavior (as far as the API is concerned), and handle it however seems appropriate.

I'm certainly curious to see what you come up with - overall I'm liking the direction your PR is heading in :-).

bighappyface · 2017-03-01T05:07:08Z

I know that the previous work done on errors was work that those folks needed. Simply put, we can't really know what folks are using, so I think we have to assume whatever is public is in use, and plan with this assumption in mind for future versions.

I put a tentative note in the 6.0.0 project for refactoring the method but we can layout whatever milestones we wish to satisfy another major version. I simply want a plan and concensus we can execute on.

shmax · 2017-03-02T22:07:59Z

Okay, open for business, again. I've changed the base to 6.0.0-dev, so go ahead and review.

erayd · 2017-03-02T22:29:10Z

src/JsonSchema/ConstraintError.php

+    const TYPE = 'type';
+    const UNIQUE_ITEMS = 'uniqueItems';
+
+    public function getMessage()


May we please also have a setMessage method that can add / overwrite messages, and store the messages on the class rather than inside getMessage? This would be useful for local translations and people's custom errors that never make it as far as upstream.

Would be good if this could accept either a single error message, or an array of them to allow setting multiple errors at once. e.g.:

public static function setMessage($id, $message) { if (is_array($id)) { self::$messages = array_merge(self::$messages, $id); } else { self::$messages[$id] = $message; } }

I think @erayd and I already came to an understanding on this, but just for those following along at home ErrorConstraint is not a localization interface. It's just a central repository for the natural language strings that we already had floating around in the code. Its only relevance to localization is as a reference.

erayd · 2017-03-02T22:35:24Z

src/JsonSchema/ConstraintError.php

+            self::UNIQUE_ITEMS => 'There are no duplicates allowed in the array'
+        );
+
+        return isset($messages[$name]) ? $messages[$name] : "Unknown Error: $name";


The unknown error should probably be in the error table with everything else - it seems neater, and there's nothing special about it to warrant being defined separately.

Hmm, tricky. It's a good thought, but then I need to figure out how to add an error for the error (so it can be localized, as well). I started working on it but I wound up with a huge blob of messy code, and it seems like a more elegant approach would be for ConstraintError to throw its own exception, which I could catch in addError (which would do its own call to itself). What do you think?

What's wrong with something like this? No reason for it to get complicated.

return isset($messages[$name]) ? $messages[$name] : $messages[self:UNKNOWN_ERROR];

Although if you want to do the exception thing, that's a perfectly workable approach too.

After thinking about it for a bit, I think that maybe throwing an UnknownErrorException and letting the user deal with it (rather than catching it) is a good idea. That way it'll cause the unit tests to fail if we ever throw a nonexistent error, and assuming we did things properly and the new code has test coverage, we should catch it before the changes ship, because Travis will be screaming at us about it.

erayd · 2017-03-02T22:39:53Z

src/JsonSchema/Constraints/BaseConstraint.php

    {
        $error = array(
            'property' => $this->convertJsonPointerIntoPropertyPath($path ?: new JsonPointer('')),
            'pointer' => ltrim(strval($path ?: new JsonPointer('')), '#'),
-            'message' => $message,
-            'constraint' => $constraint,
+            'message' => ucfirst(vsprintf($constraint->getMessage(), array_map(function ($val) {


Don't use ucfirst here, it's not multibyte-safe, and not all languages have the concept of capital letters. Better IMO to define the messages with a capital in the first place, and let anyone providing custom or translation messages handle their own casing.

Also, why do all the format stuff here? More sensible IMO to have the constructor handle it; that way you just instantiate the error with its variable components and you get a finished package, rather than post-processing it as you're doing here.

I took the ucfirst from elsewhere in the code. If it was safe before, it's safe now. Remember that this bit of code has nothing to do with localized strings. The messages managed by ConstraintError are part of our language-agnostic internal workings, which are currently in English.

Won't this bit end up handling localized strings at some point though (and isn't that a desirable thing)? If not, then you're double-handling the formatting.

If it won't ever handle localized strings, then I may not entirely understand how you envisage this working - if that's the case, can you explain an outline of how you intend things to work?

Remember that our goal is NOT to do any localizing ourselves, not now, not ever. All I'm shooting for in this PR is to:

centralize the error codes and internal error messages so that someone who DOES want to localize has an easy reference

Make the arguments that would be passed to a localizing mechanism easily available ( params in the error object), already in the correct order and ready to go.

Typehint addError somehow so that it is impossible for json-schema devs (not localizers) to misuse.

That's pretty much it. So how WOULD someone do localization? Minimally, like this:

foreach( $validator->getErrors() as $error ) { $msgId = "msg_json_schema_error_{$error['constraint']"; // insert your favorite localization technology here echo $i18n->loc($msgId, $error['params']; }

I guess I would like to hear some feedback from one of the guys that wanted localization to see what he has in mind; maybe we could find a way to add some sugar.

Aaah, I thought you were intending a localisation interface to eventually come out the far side of this, to support people who were wanting to localise while still not doing that ourselves. If you're wanting to keep the existing error format, then the way you have done it seems like a suitable approach :-).

I honestly hadn't gotten as far as making it fully glorious for real-world localizers with real use-cases and real technologies. Let's call this phase I, and invite comment from the guy who wanted help with this...

I think we should go with this as you've written it, and maybe do anything else in a different PR, because actual localization interfaces would seem to be out-of-scope for this one.

erayd · 2017-03-02T22:44:28Z

src/JsonSchema/Constraints/BaseConstraint.php

@@ -36,23 +37,28 @@ public function __construct(Factory $factory = null)
        $this->factory = $factory ?: new Factory();
    }

-    public function addError(JsonPointer $path = null, $message, $constraint = '', array $more = null)
+    public function addError(JsonPointer $path = null, ConstraintError $constraint = null, array $more = array())
    {
        $error = array(


Rather than sticking with the array format, why not roll all this into the ConstraintError class? That way you can just pass one object around, and it doesn't remain an unspecced property bag the way it is now. More reliable for anyone needing to interact with it and easier to extend. This is the sort of thing I was intending to do when I was going to overhaul the errors, but you beat me to it with this PR ;-).

ConstraintError is just an enum. I'm not sure it's really meant to be built up into a proper class, with its own members and whatnot. You don't new them, for example. You can read up on it here: https://github.com/marc-mabe/php-enum

As mentioned in the other comment, ConstraintError is not used like a conventional class. It's an enum, with a very rigid use pattern. We could throw it out and use a regular class like you're suggesting, but we would still have a lot of fragmentation of the properties passed in (although it's a little tighter now that I'm sectioning more off into the params bucket). I'm not sure that using a class would really gain us anything other than sort of formalizing the values it manages, but this is the only place in the code where the values are gathered together, so what would be the gain? A consumer would now have a class instance dropped in his lap to figure out.

I would quite like a class, but I can also see where you're coming from here too - I think we had quite different visions for how the errors should eventually end up working.

May I think on this a bit? I like what you're doing here, but I would like to see if I can think of a way to integrate the two approaches, as I see no reason why they can't work together.

Would you consider accepting a PR on your PR branch?

I was not trying to solve the error system, here. You are still welcome to do that. I was only trying to get localizers unstuck. Why don't we just evaluate this PR in light of that goal, merge it into 6.0 if it accomplishes what it sets out to do, and then you can revamp the error system in another PR all you like and no argument from me (although I will have plenty of arguments)?

Works for me :-)

erayd · 2017-03-02T22:49:09Z

src/JsonSchema/Constraints/BaseConstraint.php

+                    return $val;
+                }
+
+                return json_encode($val);


I like the idea of json-encoding complex types. Good idea 👍.

erayd · 2017-03-02T22:51:12Z

src/JsonSchema/Constraints/BaseConstraint.php

+
+                return json_encode($val);
+            }, array_values($more)))),
+            'constraint' => array(


Same comment as above - if you feed it to the constructor, then this becomes unnecessary.

erayd

Now that my prior misunderstanding of what you were trying to do is solved, I think I'm pretty happy with this.

LGTM :-).

shmax · 2017-03-04T03:10:45Z

@harej any comment?

harej · 2017-03-04T03:54:19Z

I think this is going in a good direction. The key thing will be having access to the type of error and parameters (i.e. offending inputs), using this information to construct error messages. Then we can build a package to prepare localized errors.

…

On Mar 3, 2017, at 7:10 PM, Max Loeb ***@***.***> wrote: @harej any comment? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

shmax · 2017-03-04T04:28:26Z

Wonderful, thanks much.

shmax · 2017-03-05T00:01:55Z

I opted not to try to work the error-not-found message into ConstraintError--it implies that it's a string to be localized, when really the only time it should ever be encountered is in json-schema development. It just wasn't like the others, but I am now throwing a InvalidArgumentException.

As long as we're changing the addError API, I also swapped the order of $constraint and $path, because I couldn't find any cases of a $constraint not being passed in, but I couldn't make it non-optional and not come before the ones that are optional.

shmax · 2017-03-05T02:22:44Z

@bighappyface please review and merge if you're satisfied

bighappyface

+1

* centralize errors * isolate 'more' info * throw exception for missing error message * swap args

adjenks · 2020-05-30T21:40:49Z

I want this release badly for the centralization of error codes for translating..

shmax · 2020-05-30T22:52:32Z

Good grief, has this never been released?

erayd · 2020-05-30T23:25:47Z

@shmax It was merged three years ago, and currently resides in the master branch, but is not backportable for compatibility reasons (see #368).

@adjenks If you use the master branch, you can make use of this feature.

shmax added 2 commits February 26, 2017 04:31

centralize errors

20f45e6

isolate 'more' info

b370d67

shmax closed this Feb 28, 2017

shmax deleted the centralize-error-codes branch February 28, 2017 23:10

shmax reopened this Mar 2, 2017

shmax changed the base branch from master to 6.0.0-dev March 2, 2017 22:06

erayd requested changes Mar 2, 2017

View reviewed changes

erayd approved these changes Mar 3, 2017

View reviewed changes

throw exception for missing error message

be50388

swap args

d05f90c

bighappyface approved these changes Mar 5, 2017

View reviewed changes

bighappyface added the Pending Merge label Mar 5, 2017

erayd approved these changes Mar 5, 2017

View reviewed changes

bighappyface merged commit 3dba977 into jsonrainbow:6.0.0-dev Mar 7, 2017

erayd mentioned this pull request Mar 7, 2017

Backports for 5.2.0 #368

Merged

erayd added a commit to erayd/json-schema that referenced this pull request Mar 13, 2017

Fix arguments accidentally swapped in jsonrainbow#364

ef7f28c

erayd mentioned this pull request Mar 13, 2017

Improvements to type coercion #384

Merged

bighappyface mentioned this pull request Mar 17, 2017

Lack of i18n for error messages #363

Closed

bighappyface removed the Pending Merge label Mar 17, 2017

erayd pushed a commit to erayd/json-schema that referenced this pull request Mar 22, 2017

centralize errors (jsonrainbow#364)

a0af784

* centralize errors * isolate 'more' info * throw exception for missing error message * swap args

PeterMead mentioned this pull request Nov 28, 2017

Error message for multipleOf no longer shows decimal places #468

Closed

shmax deleted the centralize-error-codes branch March 2, 2019 01:49

DannyvdSluijs mentioned this pull request Feb 26, 2024

Custom error message #239

Closed

centralize errors #364

centralize errors #364

Conversation

shmax commented Feb 26, 2017 • edited Loading

erayd commented Feb 26, 2017 • edited Loading

shmax commented Feb 26, 2017

erayd commented Feb 26, 2017

erayd commented Feb 26, 2017

shmax commented Feb 26, 2017

shmax commented Feb 26, 2017 • edited Loading

shmax commented Feb 26, 2017 • edited Loading

shmax commented Feb 28, 2017

erayd commented Feb 28, 2017

shmax commented Feb 28, 2017

bighappyface commented Feb 28, 2017

bighappyface commented Feb 28, 2017

shmax commented Feb 28, 2017

erayd commented Feb 28, 2017

erayd commented Feb 28, 2017

erayd commented Feb 28, 2017

shmax commented Feb 28, 2017

erayd commented Feb 28, 2017 • edited Loading

bighappyface commented Mar 1, 2017

shmax commented Mar 1, 2017

erayd commented Mar 1, 2017 • edited Loading

shmax commented Mar 1, 2017 • edited Loading

erayd commented Mar 1, 2017 • edited Loading

bighappyface commented Mar 1, 2017

shmax commented Mar 2, 2017

Choose a reason for hiding this comment

shmax Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmax Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

erayd Mar 3, 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

shmax Mar 3, 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

shmax Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

shmax Mar 3, 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

erayd left a comment

Choose a reason for hiding this comment

shmax commented Mar 4, 2017

harej commented Mar 4, 2017 via email

shmax commented Mar 4, 2017

shmax commented Mar 5, 2017

shmax commented Mar 5, 2017

bighappyface left a comment

Choose a reason for hiding this comment

adjenks commented May 30, 2020 • edited Loading

shmax commented May 30, 2020

erayd commented May 30, 2020 • edited Loading

shmax commented Feb 26, 2017 •

edited

Loading

erayd commented Feb 26, 2017 •

edited

Loading

shmax commented Feb 26, 2017 •

edited

Loading

shmax commented Feb 26, 2017 •

edited

Loading

erayd commented Feb 28, 2017 •

edited

Loading

erayd commented Mar 1, 2017 •

edited

Loading

shmax commented Mar 1, 2017 •

edited

Loading

erayd commented Mar 1, 2017 •

edited

Loading

shmax Mar 3, 2017 •

edited

Loading

shmax Mar 3, 2017 •

edited

Loading

erayd Mar 3, 2017 •

edited

Loading

shmax Mar 3, 2017 •

edited

Loading

shmax Mar 2, 2017 •

edited

Loading

shmax Mar 3, 2017 •

edited

Loading

adjenks commented May 30, 2020 •

edited

Loading

erayd commented May 30, 2020 •

edited

Loading