use JsonPointer for path handling #276
Conversation
|
bummer, first commit and already broke the 5.3 build - didn't check the library requirements beforehand .. sorry for that, already pushed a fix using the old array syntax |
|
@steffkes According the RFC6901 a # evaluate to the the whole document. |
| public function testJsonPointerWithPropertyPaths() | ||
| { | ||
| $initial = new JsonPointer('#/definitions/date'); | ||
| $modified = $initial->withPropertyPaths(array('~definitions/general', '%custom%')); |
There was a problem hiding this comment.
To prove withPropertyPaths is immutable. The code needs to be changed here into:
$initial = new JsonPointer('#/definitions/date');
$this->assertEquals(array('definitions', 'date'), $initial->getPropertyPaths());
$this->assertEquals('#/definitions/date', $initial->getPropertyPathAsString());
$modified = $initial->withPropertyPaths(array('~definitions/general', '%custom%'));
$this->assertEquals(array('definitions', 'date'), $initial->getPropertyPaths());
$this->assertEquals('#/definitions/date', $initial->getPropertyPathAsString());
$this->assertEquals(array('~definitions/general', '%custom%'), $modified->getPropertyPaths());
$this->assertEquals('#/~0definitions~1general/%25custom%25', $modified->getPropertyPathAsString());
There was a problem hiding this comment.
@jojo1981 i'm fine adding those two lines - would you mind explaining why you think they are necessary? i can't get my head around, what are those lines asserting that the current test doesn't? it's rather a duplication of the existing tests - which obviously doesn't hurt, but it still doesn't relate to the question if the method returns a new object or modified the existing
if we want to have additional checks, what about assertNotSame for $initial and $modified ?
There was a problem hiding this comment.
@steffkes It depends on which behavior should be tested. Asserting a new object is returned is 1 thing. asserting this has no side effect is another thing (so the original object has NOT be changed). I think both can be tested (so proved).
There was a problem hiding this comment.
@jojo1981 that still doesn't answer my question for more explanation about your intention. the second assert block for $initial already implies that it was okay in the first place - i don't see what kind of errors we could avoid by testing that (again). i'm not opposed to your suggestions, not at all, just want to understand if there is a real improvement that i'm just not seeing right now
|
i went ahead an included a |
|
anyone like to have a second look? @bighappyface, probably @mirfilip or @jojo1981 (again, if you didn't already?) the newly added |
|
@steffkes I'll schedule some time to review this in the coming days. I'm super slammed at the moment but very grateful for your work. |
|
no worries @bighappyface, no need to rush it - thanks for your feedback. let me know if you have questions and/or suggestions, i'll wait for it. spare time is sometimes rare, been there too (: |
|
@steffkes would you please rebase this on master and squash the commits down to one? |
|
@bighappyface i'm not sure that it went entirely as planned - didn't need rebasing/squashing that often in the past. if it doesn't look like it should - please do let me know. if you can i would appreciate it if you could tell me where i got it wrong. i expected the other commits to disappear from this PR after doing all the steps i found described on How-to-Rebase-a-Pull-Request. at least this is what i looks like in my local git history .. |
|
ah, looks like it's working - maybe some sort of cache on github that kept the older commits for the time being. the only commit i can see now is the latest that was created during the whole rebase/squash operation. |
|
Squash looks good |
|
gentle reminder, no rush here - if you need anything else from me, drop me a note @bighappyface |
|
@steffkes my apologies for yet another rebase on master but some changes in another PR have put this into conflict. Would you mind rebasing on master once more? You shouldn't have to worry about squashing anymore. |
|
@bighappyface i should have checked that before - thanks for the heads up. did another rebase - the diff looks good again. if you need anything else, let me know |
|
@steffkes no worries. Would you mind reviewing the test failures? |
|
argh, should have done that before as well .. will do it now -- should be working again, will wait for the tests to be finished ... |
|
working again @bighappyface |
|
@jojo1981 @mirfilip @steffkes @FlorianSW would you mind taking a look at this PR? I am thinking this would get bundled into a major version release to avoid any BC issues. |
| { | ||
| $validator = $this->getFactory()->createInstanceFor('enum'); | ||
| $validator->check($value, $schema, $path, $i); | ||
|
|
||
| $this->addErrors($validator->getErrors()); | ||
| } | ||
|
|
||
| protected function checkFormat($value, $schema = null, $path = null, $i = null) | ||
| /** | ||
| * Checks format of a element |
|
anyone? anything else? :) |
|
+1 |
crafting a new API, trying to follow the jsonapi-specs, i've realized that error-objects require a json pointer:
before making more changes, first thing to do was to get all tests working again, especially bringing back all the existing property paths since i don't intend to introduce a large BC which i probably still do? :/
having had a look at RFC6901 again, i'm curious why
JsonPointer#getPropertyPathAsStringdoes include a leading hashmark @jojo1981 - isn't that actually related to the filename rather than the path? checking other existing JsonPointer implementations in PHP, all use path expressions starting with a slash instead.as a nice side-effect, using dots in keys wouldn't result in path that is rather hard to understand like
tld.company.project[0].content(instead of/tld.company.project/0/content)@mirfilip @bighappyface WDYT?