halaxa / json-machine

Efficient, easy-to-use, and fast PHP JSON stream parser
Apache License 2.0
1.1k stars 65 forks source link

Only complete pointer paths now considered intersecting #107

Closed XedinUnknown closed 12 months ago

XedinUnknown commented 1 year ago

This fixes halaxa/json-machine#106 by adding a path separator to terminate each pointer when comparing for intersection.

mad-briller commented 1 year ago

i ran into this issue recently and this would be a great change if it works as anticipated

however the tests in this PR only test the outcome of the validator and not the actual parser when used with intersecting pointers, perhaps that would be a good thing to also test?

it may be that the algorithm simply will not handle intersection of any type in the keys i'm not super knowledgable on the parser itself and assume you have done some testing of the outcome locally however?

XedinUnknown commented 1 year ago

Hi @mad-briller! I've done testing, sure, and it works for me in a project.

Regarding parser testing, I'm not sure if it's actually a part of the parser's concern. But if it is, I'm also not that knowledgeable about parsers. In fact, there are #105 and #108 that would need someone with parser experience to fix. If you'd like, I can give you write access to the repo with the fix, and feel free to add more tests.

codecov-commenter commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (1b3aa89) 100.00% compared to head (1581575) 100.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #107 +/- ## =========================================== Coverage 100.00% 100.00% Complexity 184 184 =========================================== Files 17 17 Lines 526 526 =========================================== Hits 526 526 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.