Add new config option, refactor coerce feature to use this, and simplify the public API #351

erayd · 2017-01-19T12:00:49Z

Why

The current API isn't particularly good at exposing new functionality to users...
...and I want to add new functionality (PR Add option to apply default values from the schema #349 and PR Option to throw exceptions when an error is encountered #354) and expose it in a sane way.
Passing $coerce all over the stack is risky - it's easy to lose track of it when making other changes.
The current design mandates exposing internal method arguments ($path and $i) to users.

Changes

Split the Constraint class to make Validator independent from it
Add Validator::validate() as the main entry point
Turn Validator::coerce() and Validator::check() into aliases
Add Factory::setConfig(), getConfig(), addConfig() & removeConfig()
Make type-coercion a checkMode option, don't pass $coerce everywhere
Add some extra tests

erayd · 2017-01-19T12:02:30Z

I'm aware that I need to write some tests that cover the new bits - I'm working on that now!

bighappyface · 2017-01-19T12:06:53Z

Quick tip: tests and new functionality come out better when the tests come first.

erayd · 2017-01-19T12:17:16Z

You make an excellent point! Although it's worth noting that this PR isn't new functionality so much as it's just rearranging a lot of stuff. Almost all of it is already covered by existing tests.

erayd · 2017-01-19T12:53:28Z

src/JsonSchema/Validator.php

+     */
+    public function validateImmutable($value, $schema = null, $checkMode = self::CHECK_MODE_NORMAL)
+    {
+        $checkMode &= $this->getCheckModeImmutableMask();


Should this throw an exception if $checkMode contains something that wants to modify $value, or just silently switch off the offending options?

Currently, it just switches them off, as this behavior is the closest to how the old check() method works in assoc mode. But I must admit, I would really rather have an exception here.

Why is this necessary? Isn't it enough that the default $checkmode is CHECK_MODE_NORMAL? I would think that as long as all of your helpers put things back the way they found them that this kind of thing is not needed...

It's there because there needs to be some kind of entry point for pass-by-value, and not enforcing immutability results in inconsistent behavior between object mode and assoc mode, because objects are never passed by value. Having the mask means that no matter what the user decides to feed it for $checkMode, the result will be the same regardless of whether $value is an object or an array.

Still not following. What exactly is the use case? How is calling this different from calling validate with no flags?

The use-case is being able to pass $value as not-a-reference. If we don't care about that, then validateImmutable can go; there's no other reason to having it. Thoughts?

The immutable mask thing is only to enforce consistency between assoc and object modes, and if this method goes then it's a non-issue.

Well, you already know my opinion. :)

OK then - I'll get rid of validateImmutable :-).

shmax · 2017-01-20T03:03:01Z

src/JsonSchema/Constraints/ConstraintInterface.php

@@ -59,5 +59,10 @@ public function isValid();
     * @param mixed            $i
     * @throws \JsonSchema\Exception\ExceptionInterface
     */
+    public function _check(&$value, $schema = null, JsonPointer $path = null, $i = null);


Isn't it a little weird for a public method to have the _ prefix?

It's not part of the public API (hence why it's prefixed with _, as is convention for internal stuff). It needs to be designated a public method because it gets called from all over the place within the library, but not just from class descendants.

Well, right, but this isn't a situation where you have to rely on the underscore convention to suggest visibility (like we often do in javascript), because we have actual visibility keywords. I think most people would agree it's sort of poor form to make a method public, but suggest that client code shouldn't call it with the underscore convention. Either it's private and you can't call it, or it's public and you can. Take a stand!

This method cannot be made private (or protected), because the entire library will break as a result. The various internal callers need a public method to talk to; that's just the way the library is architected.

I agree that having it actually be a public method is undesirable, but unless you have a suggestion for how to mitigate that requirement, then I think it's just something we need to live with, and all we can do is make it look as undesirable as possible for an end-user to rely on. Re-architecting the entire library to change a single visibility keyword seems like a big waste of time - it seems reasonable in this case to just be pragmatic and leave it alone, albeit well-documented as an internal method.

I guess another option would be moving the contents of Validator::check() into validate(), and leaving _check() as an empty method (it can't be removed without also removing it from the interface, which is an important sanity check for those writing constraints). I don't much mind either way - would having Validator::_check() be a noop be preferable for you, as opposed to the status quo?

Nah, I'm happy to do it, seeing as my wanting to add the defaults PR is the source of this whole headache - it seems reasonable that if I want that feature, it should be my time that gets used making it happen. Code review is always appreciated though :-).

🥇 By the way, just so there's no misunderstanding, I am not a project maintainer--just the most vocal contributor these days.

Gotcha. Do you know whether the maintainer is likely to agree with you on this? Is there any chance of us getting their opinion on the situation?

Vocal means you care, and if you're the only contributor with a strong opinion, and the maintainer is OK with that, then I'm happy to defer to your preference somewhat.

@bighappyface Is the only active contributor with write privileges. He's usually pretty mellow, but no harm in getting his opinion now.

Have not heard anything from @bighappyface, so I'm just going to go ahead and implement this as discussed above (one top-level method Validator::check(), all others to go, refactor so that _check is not public).

shmax · 2017-01-20T03:08:03Z

src/JsonSchema/Validator.php

+
+    /**
+     * Validates the given data against the schema and returns an object containing the results
+     * Both the php object and the schema are supposed to be a result of a json_decode call.


Why would you make a point of mentioning json_decode? There are other ways to come up with an object...

$schema = (object)['foo'=>'bar']; $schema = new stdClass(); $schema->foo = 'bar';

I didn't - this is verbatim from master (courtesy of @digitalkaoz 5 years ago). I saw no reason to change it, as it explains the intended use-case quite well.

I find it misleading. Not everybody is decoding json (I'm not--all of my schemas come from php files).

Can certainly change it... what would you like to see instead?

Don't know. Might be enough to just remove the comment about json_decode, and document the mixed type for $value?

That works - I'll do that.

shmax · 2017-01-20T03:18:24Z

src/JsonSchema/Validator.php

+     *
+     * {@inheritDoc}
+     */
+    public function validate(&$value, $schema = null, $checkMode = self::CHECK_MODE_NORMAL)


I like the idea that you have sort of an advanced usage method where you can explicitly state the kinds of bells and whistles you want. I would argue that this should be the main entry point, and that everything else (coerce, check, forceDefaults, etc) could be considered helpers, shortcuts, or possibly even eventually deprecated and removed entirely.

One suggestion you might consider is to convert all instances of _check (which is confusingly public and mirroring check) with validate, and just pass $checkmode along everywhere. The next step after that would be to think about removing the internal $checkmode and make this whole thing completely stateless.

The validate() method is supposed to be the main entry point for everything except pass-by-value calls, which use validateImmutable() - so not sure what you're disagreeing with here? check() and coerce() are intended to be deprecated; the only reason they're still around is to avoid breaking things for people who are currently using them.

I would prefer to avoid conflating validate() with _check() - they're intended to be different things. Having the public API be the same thing as all the internal stuff seems rather messy IMO - that's what happened with coerce(), and half the point of this PR is to remove any need for the 'pass-everything-down-the-stack' approach. Unless you're suggesting we make every part of this library thread-safe, then going back to passing stuff around seems like a backwards step.

I'm not disagreeing, really, just trying to help massage this into something a little more structurally solid. Well, I guess I do disagree with validateImmutable--adding a method whose sole purpose is to ignore certain flags you pass in seems a little bizarro-world to me--but otherwise I like the general tack you're taking.

Really, I was trying to give you an elegant way to avoid the corner you've painted yourself into with _check. Before you started all this we had two single public methods at the top level. I suggested reducing them to one (yes, that would conceivably break backwards compatibility for what is probably a fairly small set of people, but we're not shy about doing that in this project if the rationale is strong, and I think this qualifies). But now, instead of just check, we now have check. validateImmutable, coerce, _check, and validate. What happened!?

Hmm.... maybe we see this a little differently? I don't feel like this is a corner.

We went from two public methods (check and coerce) to two public methods (validate and validateImmutable), and in doing so significantly improved the interface from an end-user perspective, plus allowing for easy addition of new functionality in the future without needing to change the public API again.

If you don't care about pass-by-value at all, then validateImmutable can be removed; that's the entire reason it's there.

If you don't care about breaking backwards-compatibility, then check and coerce can certainly be dropped... but IMO keeping them is safer - they're just aliases now anyway. Perhaps having them trigger a deprecation notice when called would be a sensible middle-ground, and then remove them in a future release?

_check is not a public method, whatever its visibility keyword says. The only reason it exists at all is because ConstraintInterface requires it. Would making _check be a noop be an improvement? Because the content of that method can easily be moved into validate.

I count two public methods on ConstraintInterface, check and _check, and three more on Validator (which is the public face of the app, and formerly was a vanilla override of Constraint), coerce, validate and validateImmutable. You're arguing that you are making things simpler, but we've gone from 2 publics on the main app instance to 5, and I barely understand it and I'm trying my honest best to keep up.

You have my endorsement to just change check. If you do it right most of the consumers of this app will never know the difference, and that'll take us down to one public method that can take all comers. We can gather some opinions from other people, but I feel like that will be much, much easier to sell than all this other stuff.

Gonna take a dinner break. Overall I'm liking the energy. Hang in there.

erayd · 2017-02-02T03:46:38Z

@shmax - have redone this PR more along the lines we discussed - there's now just one public method defined in Validator. Thoughts?

I've opted to use a config array rather than just $checkMode, in order to future-proof things in case somebody wants to add a feature that can't be configured via a partial bitmask. That way the risk of needing to change the public API again in order to support new features is minimised.

shmax · 2017-02-02T04:18:03Z

I think it's a huge improvement in a lot of ways; I like that all those $coerce parameters can go away, and I really like that you got us back down to a single public method, and I love that you abstracted BaseConstraint out so that the top level check doesn't have all that weird internal $i stuff on it (I've been wanting to do something similar). I can't say I'm a fan of the config property bag idea, however, for a few reasons:

You see property bags all the time in javascript, but it's not as common in PHP. I don't know of any native functions that do it, but there are definitely a few that use the bitmask style.
One nice thing about using the bitmasks and consts defined on classes is that you can discover them via intellisense in most modern IDEs.
this will really break backwards compatibility. Your point about having a little more flexibility for future changes is fair and well-taken, but my instinct is that we can cross that bridge if and when we come to it, and that this much of a change (meaning changing checkmode to an associative array) might be hard to sell without something a little more concrete to support it than "we might need it someday". But if you're dead set on it I'm willing to defer to other reviewers.

What do you think?

erayd · 2017-02-02T05:46:00Z

Can you clarify what your issue is with having that $config array? It would be relatively easy for me to refactor that bit of the PR to work differently, but I guess I don't really understand where you're seeing it as being a problem, and I don't want to just guess. Would you prefer that the config have its own class, rather than being an array?

I do feel fairly strongly that a bitmask alone is insufficient - but a bitmask is definitely still useful, which is why I turned checkMode into a config property, rather than completely getting rid of it.

Re backwards compatibility - yes, this definitely breaks compatibility. I thought that you decided that did not matter - but I'm quite happy to keep some backwards compatibility if you let me know which bits of the API you feel need to be not-broken so that I can ensure they maintain compatibility. checkMode does still exist as a config property, and it would be very easy to expose that in the places you feel that it should remain part of the public API.

erayd · 2017-02-02T05:59:30Z

@shmax Further to my comment above, one of the reasons I made coerce & apply-defaults as independent booleans rather than being part of the checkMode bitmask was to make them easy to set independently. This isn't a fire-and-forget thing the way most PHP bitmask configs are (because Factory can be reused across multiple validations), so it makes sense to allow the user to adjust individual settings without requiring them to do a bunch of extra bitwise operations just to avoid clobbering unrelated settings.

shmax · 2017-02-02T06:05:13Z

Sure, I'll take another stab at explaining my position: I liked it better before when $checkMode was just an int, and turning coercion on was a matter of applying CHECK_MODE_COERCE (that might have been before your time--I eventually removed it when I added the coerce method, but now we're in a position to put it back). Since we don't currently have any features or options that can't easily be handled with the bitmask, my vote is to stick with it.

I didn't say that breaking compatibility doesn't matter--I said we're not shy about doing it when the situation warrants it. The config bag thing is not a must-have--it's more like a personal coding style that you favor, and it doesn't buy us anything in the short term (and we're a little hazy on the long term). To be fair, changing check to take a reference also breaks compatibility for some people (I started poking through projects at random on github--more people than I would have expected do use json_decode inline with check, as you warned), but it makes two major features that you and I want (apply defaults and coerce) much more doable, and adapting to the change is trivial (not that yours is that much of a big deal, but people would have to look at the documentation).

Really, I think everything you did is great, but if it were me I would get rid of the property bag, put $checkMode back the way it was (as a top-level argument to Factory), and re-add CHECK_MODE_COERCE.

erayd · 2017-02-02T06:06:02Z

@shmax Also further to your point re future-proofing - it makes sense to me that if we are breaking the API anyway (which we are, because Validator::coerce() is going away and Validator::check() now requires a reference, we might as well do it properly, once, rather than making the API too limited now and potentially breaking it again later - because every time the API breaks and requires users to refactor stuff, that will annoy them.

[ed: posted this before I saw your most recent comment - it's probably redundant now :-)]

erayd · 2017-02-02T06:08:28Z

@shmax Give me a moment, and I'll see if I can make a few changes that will bring things more in line with what I think you're after.

shmax · 2017-02-02T06:08:44Z

Righto, sounds good.

shmax · 2017-02-02T06:20:23Z

Just a quick response to your comment about Validator::coerce going away--that's true, but I don't think too many people have started using that, as it's still a new addition. I did a quick search on github and only turned up 25 results, and of those the first 5 or 6 I checked were people that had added their vendor directory to their repo for some reason, and the match was coming from CHECK_MODE_COERCE (which has been around for longer).

shmax · 2017-02-02T06:28:20Z

Hmm, I just realized something. Now that you've divorced Validator::check from Constraint::check (which, again, is awesome), we can pass $checkMode along as a third argument. That wouldn't break compatibility because existing code will only be using two arguments (with the third and fourth currently being $path and $i, which don't really have meaning from the top entry point), AND you then have true "fire and forget" functionality, with more of a one-liner feel (instead of having to keep a Factory instance around so that you can twiddle flags on it)...

shmax · 2017-02-02T06:35:38Z

Oh, damn, I see that you already are passing ~~~$checkMode~~~ $config as a third argument. My apologies--now I understand some of your bafflement. I'd better get some sleep before I make things any worse, but I'll be ready to resume the discussion tomorrow, if that's okay with you.

shmax · 2017-02-02T06:55:27Z

Heading to bed, but I think it's looking great--we're almost there. More soon.

erayd · 2017-02-02T06:56:37Z

@shmax OK, so I've done the following - thoughts?

Renamed the new API to Validator::validate();
Added a Validator::check()->Validator::validate() alias to maintain backwards-compatibility for people who use pass-by-value;
Restored $checkMode as the third argument to Factory::__construct();
Added $checkMode as the third argument to Validator::validate() ($config moves to fourth);
Defined bitmask flags CHECK_MODE_COERCE_TYPES and CHECK_MODE_APPLY_DEFAULTS;
If $checkMode is set on Validator::validate(), then those flags will override the config options.

These changes preserve much more backwards-compatibility, still keep a simple API, allow fire-and-forget via $checkMode flags, and still keeps the config array around so that factory reuse and bitmask-incompatible features remain easy. Does this sufficiently address the concerns you had earlier?

I see your heading-to-bed comment - sleep well :-).

erayd · 2017-02-02T08:02:15Z

Do you want an alias for Validator::coerce() as well? Or prefer to take this out regardless, and not worry about backwards-compatibility for those users?

It doesn't need to be more than a one-liner:

public function coerce(&$value, $schema)
{
    return $this->validate($value, $schema, null, array('coerce-types' => true));
}

shmax · 2017-02-02T18:01:02Z

Had a busy morning and now I need to get to work, but I'll get back to you tonight...

shmax · 2017-02-03T05:52:31Z

So it's looking good, but I don't quite understand why checkMode is expected as both/either the 3rd argument to validate AND/OR as a property on $config, (AND you have both CHECK_MODE_COERCE and "coerce-types" and CHECK_MODE_SET_DEFAULTS and "set-defaults"). It seems to me that if we've managed to agree on $checkMode as the 3rd argument, then there's not much need to support it on $config. And that leaves $config empty, which means it doesn't really need to be there, right?

If you ever do come up with a complex config option that can't be handled with $checkMode, then you can re-introduce $config at that time as the 4th argument without breaking compatibility.

erayd · 2017-02-03T06:22:47Z

I mainly put $checkMode as the third argument because it seemed like something that was important to you - I wouldn't go so far as to say I agree with it, but it seems to me that you care about it a lot, so I'm trying to come up with a compromise. It sounds like you're not entirely happy with the latest suggestion, so I'll tweak it again - stand by...

erayd · 2017-02-03T07:03:21Z

@shmax How about now?

The biggest point in favour of having a config array now, IMO, is the ability to change options individually without requiring the user to do a bunch of bitwise operations just to avoid clobbering the settings they're not changing. I guess I'm not really understanding why you have such a strong resistance to it - does the latest lot of changes implement it in a way you find acceptable, or am I missing something fundamental about your objection to it?

~~No idea what you were talking about with CHECK_MODE_SET_DEFAULTS / set-default; those never existed in any of my code. Can you clarify?~~ Never mind, apparently I just missed the obvious - you were talking about the apply-defaults feature.

shmax · 2017-02-03T07:49:02Z

Hmm, interesting. I'll have to take a closer look at this tomorrow; but here's a few reasons why I object to your property bags idea in this specific case, and a few why I don't like them in general in PHP:

you can't typehint the properties. Best you can do is typehint the object itself. You've been implying that the config object will come in handy if we start dreaming up more complex config properties, but then I would argue that you're making a bad pattern worse because the more complex a property is, the more useful a typehint might be.
if we stick with a bitmask and flag constants, it's very difficult to misuse. Most IDEs will help you discover the available consts with intellisense, but even if you don't make use of that feature any mistyped flags will certainly get caught at runtime. The same is not true of strings like "coerce-types". If you mistype it as "coerce-type", it will simply fail silently.
It's not a very PHP-y kind of thing, at least not as an argument to a method. You see it all the time in javascript, and I'll confess I do it in my own code sometimes (usually when constructing widgets with a lot of config properties), but I almost never see it out in the wild. I'm not arguing that people won't be able to understand it or something, just that it's not a popular pattern, and it makes me suspect that there are probably other reasons not to do it that I haven't even thought of.

But anyway, let me take another look at it tomorrow with fresh eyes. G'nite.

shmax · 2017-02-03T07:50:46Z

the ability to change options individually without requiring the user to do a bunch of bitwise operations just to avoid clobbering the settings they're not changing

One last comment; I thought we had dodged this problem by passing checkMode in as an argument on every call to validate? Or is it still storing the value of checkMode internally until the next call?

erayd · 2017-02-03T07:53:28Z

Thanks for explaining :-). Your point about typehinting is a really good one, and not something I'd considered. So is the thing with typos - at least with constants, you get an error if you typo it.

Noting those two points, I think you've convinced me to ditch the config array. I still care about being able to set individual options, but that's doable with checkmode alone if I tweak it a bit. Stand by for more changes...

One last comment; I thought we had dodged this problem by passing checkMode in as an argument on every call to validate? Or is it still storing the value of checkMode internally until the next call?

It's still stored internally on Factory - Validator::validate() just saves and resets it to allow fire-and-forget using different settings.

erayd · 2017-02-03T08:33:16Z

OK, what about this?

$config is completely gone;
$checkMode is the sole means of configuring features;
$checkMode is still stored internally in Factory to allow reuse;
Validator::validate() saves and restores the existing $checkMode to allow fire-and-forget;
Factory::setConfig() can (avoids the user needing to manually preserve settings):
- set the entire $checkMode bitmask;
- add options to the current $checkMode;
- remove options from the current $checkMode.
Factory::getConfig() can:
- get the entire $checkMode bitmask;
- get a subset of the $checkMode bitmask.

shmax · 2017-02-04T07:25:04Z

Still with you... had a busy day, but I'll come back to this tomorrow. Thanks for hanging in there...

shmax · 2017-02-04T17:11:06Z

src/JsonSchema/Constraints/Constraint.php

-        $this->errors = array();
-    }
+    const CHECK_MODE_COERCE_TYPES =   	0x00000004;
+    const CHECK_MODE_APPLY_DEFAULTS =  	0x00000008;


can you line the numeric literals up so that they're in a neat column?

Sorted - looks like there were tabs in there that shouldn't have been, and I have vim configured to display tabs using a 4-char tabstop, so I didn't see them. Well spotted!

shmax · 2017-02-04T17:16:54Z

src/JsonSchema/Constraints/Factory.php

+     * @param int $checkMode Set checkMode options
+     * @param bool $enable Whether to enable or disable the provided options. If null, overwrites checkMode.
+     */
+    public function setConfig($checkMode = Constraint::CHECK_MODE_NORMAL, $enable = null)


I find this a little confusing. If I pass in $checkMode but $not enable, then I overwrite the whole thing. Otherwise, you either add or remove the flag, dependent on $enable. Do you really think people are going to be busily setting and unsetting specific flags over the course of a program? Wouldn't it be simpler to just drop the $enable arg and just make this a simple set like you have on line 92? If someone want to unset a flag, they would do so by just setting the exact config they want and blowing away the old config...

And if you really want the ability to add and disable specific flags, maybe you could add addConfig and removeConfig helpers

$enable is now gone, and setConfig() will always overwrite. I do really want to keep the ability to enable / disable specific flags, so have added addConfig() and removeConfig() per your suggestion.

Do you really think people are going to be busily setting and unsetting specific flags over the course of a program?

Not so much that - I had two specific use-cases in mind:

If a flag is default-enabled internally, and the user doesn't know that, but wants to set a flag of their own - addConfig() is clearly safer in that situation.

If a user wishes to disable a single default-enabled flag without messing up whatever else might be in $checkMode.

At the moment the default is just CHECK_MODE_NORMAL, but that may not always be the case.

shmax · 2017-02-04T17:23:01Z

tests/Constraints/FormatTest.php

@@ -23,7 +23,8 @@ public function testNullThing()
        $validator = new FormatConstraint();
        $schema = new \stdClass;

-        $validator->check('10', $schema);
+        $checkValue = 10;
+        $validator->check($checkValue, $schema);


Just curious; is it still necessary to pass in a variable now that top-level check no longer takes a reference?

Yes, because the individual validators do require a reference. The references are needed for anything that modifies the input value, including type coercion and setting default values.

The line you've highlighted here tests FormatConstraint directly, not the top-level Validator class.

Ah, ok. Sounds good.

shmax · 2017-02-04T17:26:30Z

src/JsonSchema/Validator.php

    {
+        $initialCheckMode = $this->factory->getConfig();
+        if ($checkMode !== null) {


so if I understand correctly, validate bulldozes whatever is already set in the internal checkMode, and, respects the incoming $checkMode, then restores it afterward. If that's true, then shouldn't $checkMode default to CHECK_MODE_NORMAL, and always call $this->factory->setConfig whether $checkMode (the arg) is set or not?

No. validate() only clobbers checkMode if the user actually passes it. If $checkMode is left as null, it uses whatever was already set on the factory. That is why the actual default value isn't set by this method.

The actual default is CHECK_MODE_NORMAL - it's set as the third argument to Factory::__construct().

Okay. I think I can live with that.

shmax · 2017-02-05T01:43:33Z

Are you ready for it..? 🥁

LGTM!

Beautiful work. Sorry for being such a pain.

erayd · 2017-02-05T02:07:19Z

Hooray! Thanks for all the time you've spent reviewing this :-).

You weren't a pain - you were thinking through the ramifications of the changes from a different perspective to mine, and it was useful - I think the end result is much better because of all your input.

Does anyone else need to review before merge / do we need to flag anyone to get this merged?

This commit makes the following changes: * Split the Constraint class to make Validator independent from it * Add Validator::validate() as the main entry point * Turn Validator::coerce() and Validator::check() into aliases * Add Factory::setConfig(), getConfig(), addConfig() & removeConfig() * Make type-coercion a checkMode option, don't pass $coerce everywhere * Add some extra tests

erayd · 2017-02-05T02:19:21Z

Squashed into 2159893, because the commit history for this PR was full of in-progress junk that's no longer applicable.

shmax · 2017-02-05T02:24:30Z

Well, at this point @bighappyface needs to look it over. He might want to get a few more eyes on it...

@digitalkaoz please review

bighappyface

+1

shmax · 2017-02-15T17:16:41Z

🎉 Thanks @bighappyface and congrats @erayd

erayd · 2017-02-15T20:40:30Z

Woohoo! Thanks guys :-D

erayd mentioned this pull request Jan 19, 2017

Add option to apply default values from the schema #349

Merged

erayd commented Jan 19, 2017

View reviewed changes

shmax reviewed Jan 20, 2017

View reviewed changes

erayd force-pushed the checkmode-and-coercion-refactor branch from 8ba2f94 to 0491e13 Compare February 2, 2017 03:43

erayd changed the title ~~Add new methods to set checkMode directly, and refactor coerce feature to use this~~ Add new config option, refactor coerce feature to use this, and simplify the public API Feb 2, 2017

erayd force-pushed the checkmode-and-coercion-refactor branch from 5fa3c19 to e0c601a Compare February 2, 2017 08:35

shmax reviewed Feb 4, 2017

View reviewed changes

erayd mentioned this pull request Feb 5, 2017

Option to throw exceptions when an error is encountered #354

Merged

erayd force-pushed the checkmode-and-coercion-refactor branch from 75a109e to 2159893 Compare February 5, 2017 02:16

bighappyface self-requested a review February 5, 2017 15:53

bighappyface added the Needs Review label Feb 5, 2017

bighappyface approved these changes Feb 15, 2017

View reviewed changes

bighappyface merged commit 9b6ebfe into jsonrainbow:master Feb 15, 2017

erayd deleted the checkmode-and-coercion-refactor branch February 15, 2017 20:17

Add new config option, refactor coerce feature to use this, and simplify the public API #351

Add new config option, refactor coerce feature to use this, and simplify the public API #351

Conversation

erayd commented Jan 19, 2017 • edited Loading

Why

Changes

erayd commented Jan 19, 2017

bighappyface commented Jan 19, 2017

erayd commented Jan 19, 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

erayd Jan 20, 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

shmax Jan 20, 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

shmax Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

erayd commented Feb 2, 2017

shmax commented Feb 2, 2017 • edited Loading

erayd commented Feb 2, 2017

erayd commented Feb 2, 2017

shmax commented Feb 2, 2017

erayd commented Feb 2, 2017 • edited Loading

erayd commented Feb 2, 2017

shmax commented Feb 2, 2017

shmax commented Feb 2, 2017 • edited Loading

shmax commented Feb 2, 2017 • edited Loading

shmax commented Feb 2, 2017 • edited Loading

shmax commented Feb 2, 2017

erayd commented Feb 2, 2017

erayd commented Feb 2, 2017

shmax commented Feb 2, 2017

shmax commented Feb 3, 2017 • edited Loading

erayd commented Feb 3, 2017

erayd commented Feb 3, 2017 • edited Loading

shmax commented Feb 3, 2017 • edited Loading

shmax commented Feb 3, 2017

erayd commented Feb 3, 2017 • edited Loading

erayd commented Feb 3, 2017

shmax commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmax Feb 4, 2017 • edited Loading

Choose a reason for hiding this comment

erayd Feb 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmax commented Feb 5, 2017

erayd commented Feb 5, 2017

erayd commented Feb 5, 2017 • edited Loading

erayd commented Jan 19, 2017 •

edited

Loading

erayd commented Jan 19, 2017 •

edited

Loading

erayd Jan 20, 2017 •

edited

Loading

shmax Jan 20, 2017 •

edited

Loading

shmax Jan 20, 2017 •

edited

Loading

shmax commented Feb 2, 2017 •

edited

Loading

erayd commented Feb 2, 2017 •

edited

Loading

shmax commented Feb 2, 2017 •

edited

Loading

shmax commented Feb 2, 2017 •

edited

Loading

shmax commented Feb 2, 2017 •

edited

Loading

shmax commented Feb 3, 2017 •

edited

Loading

erayd commented Feb 3, 2017 •

edited

Loading

shmax commented Feb 3, 2017 •

edited

Loading

erayd commented Feb 3, 2017 •

edited

Loading

shmax Feb 4, 2017 •

edited

Loading

erayd Feb 5, 2017 •

edited

Loading

erayd commented Feb 5, 2017 •

edited

Loading

shmax commented Feb 5, 2017 •

edited

Loading

erayd commented Feb 15, 2017 •

edited

Loading