search

wanglingsong / JsonSurfer

A streaming JsonPath processor in Java
MIT License
292 stars 55 forks source link

Allowed filter to be followed by relative path #49

Closed mthaak closed 5 years ago

mthaak commented 5 years ago

As it's my first public pull request I'm open to feedback!

mthaak commented 5 years ago

This followed from #33.

wanglingsong commented 5 years ago

Thanks so much for your contribution! However, as I said, I won't be so easy to implement this feature if you just bypass the antlr syntax. Much more works required in refactoring jsurfer-core module. I just made some modification on your new test case, you can check how it fails.

mthaak commented 5 years ago

Ah too bad, the new tests fail indeed. I already suspected it was too easy to be correct haha.
For me it's however not clear why a path after a filter doesn't work while it does work for children ['<name>' (, '<name>')], indexes [<number> (, <number>)] or slices [start:end].
Also it's not clear for me where in the code I would have to change this (and how much work this would take).

mthaak commented 5 years ago

I stepped through the code and I found it's indeed quite a big change to allow multiple filters or filters in the middle of a JSON path. The reason is that in your JsonPath implementation you only maintain a single JsonPathFilter that you apply at the end of the listener chain (i.e. you don't prune midway if the filter doesn't apply). Now I am still unsure why you chose this implementation though.. but I will probably not change it anytime soon because it doesn't have too high priority for us and requires quite a bit more work considering I don't fully understand the code base yet. But besides this I do like your implementation and it's clearly a piece of craftsmanship.

wanglingsong commented 5 years ago

Thanks! Actually, This implementation was a workaround for the minimal use case of JsonPath filter, of course not perfect but it works and I did state this limitation in Readme.

mthaak commented 5 years ago

I cracked at it for a few days. It seems to work now. See if you like it

wanglingsong commented 5 years ago

Thanks so much for your hard work! It looks like a huge change. I need some time to investigate the impact. Will try to get back to you asap.

mthaak commented 5 years ago

I hope you are managing to understand these changes. If not, shoot me your questions.

wanglingsong commented 5 years ago

Sorry for the late reply. I just commited a new implementation which is revised from your code. Please check and see if any question.