cerbero90 commented 2 years ago

Hey @halaxa! Long time since our last PR! 😄

Your package keeps improving, I love the support for multiple JSON pointers ❤️

While updating my package (based on yours), I noticed that some changes are interrupting the iteration of nested sub-trees at the first matching JSON object and ignore the remaining iterations. Which appears to be the issue flagged here: https://github.com/halaxa/json-machine/issues/78

This PR aims to bring the following changes:

let indexes of JSON arrays be integer within the current path. This brings 2 benefits:
- accurate current path, JSON array indexes are actually integers
- avoid the use of regular expressions to perform checks against parts of the path
fix how we compute the current path and current path wildcard
- currently these paths are not cleaned when the current level drops by 2 or more levels
fix the parsing of nested sub-trees that use wildcards
- currently they are interrupted after the first iteration
simplify and make the logic to compute the matching JSON pointer more accurate
improve and add one more test for parsing nested sub-trees using wildcards

halaxa commented 2 years ago

Hi, nice to see you again :) I'll be able to engage in about a week. Thank you very much for pushing it further.

(To be honest, I intend to rewrite the whole Parser from scratch, it's getting a little messy. But it's hard to say when I get to it...)

cerbero90 commented 2 years ago

Hi @halaxa, no worries :) Do you plan to rewrite the parser before tackling this PR or you are thinking of rewriting it at a later stage?

halaxa commented 2 years ago

I'm sorry for not replying. I'm currently a little busy with some voluntary work. I'm not sure when I get to this PR. But at the first look, I like that some of the messier code is gone 👍. In the meanwhile, could you rename "json pointer segment" to "reference token" for less confusion? It's the official designation: https://datatracker.ietf.org/doc/html/rfc6901#section-3. Thanks ;)

halaxa commented 2 years ago

Although I know you didn't break it, making the build work could be a valuable meanwhile-contribution as well 😁.

cerbero90 commented 2 years ago

No worries at all, take your time :)

I renamed the variables as you suggested, I guess the issue with the build is related to the conflict between the versions of PHP and composer:

Composer 2.3.0 dropped support for PHP <7.2.5 and you are running 7.0.33

You may consider to either add matrices to the build script or drop support for old versions of PHP

halaxa commented 2 years ago

Merged the build fix from master with the fix for the composer version. make build works now.

codecov-commenter commented 2 years ago

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:

Coverage data is based on head (50aeebe) compared to base (21057a6). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #83 +/- ## =========================================== Coverage 100.00% 100.00% - Complexity 181 184 +3 =========================================== Files 17 17 Lines 528 526 -2 =========================================== - Hits 528 526 -2 ``` | [Impacted Files](https://codecov.io/gh/halaxa/json-machine/pull/83?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Filip+Halaxa) | Coverage Δ | | |---|---|---| | [src/Parser.php](https://codecov.io/gh/halaxa/json-machine/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Filip+Halaxa#diff-c3JjL1BhcnNlci5waHA=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Filip+Halaxa). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Filip+Halaxa)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

cerbero90 commented 2 years ago

Thanks for finding the time to review and merge the PR! Do you reckon this fix can be tagged as 1.1.3? :)

halaxa commented 2 years ago

Sure. I was just waiting for your response and maybe for #78 's as well.

halaxa / json-machine

Fix the parsing of nested sub-trees that use wildcards #83

Codecov Report