-
-
Notifications
You must be signed in to change notification settings - Fork 197
- Fix: wrap: false returning inconsistent data types#111
- Fix: wrap: false returning inconsistent data types #111brettz9 merged 2 commits intoJSONPath-Plus:master from
Conversation
485bd8d to
6762a51
Compare
6762a51 to
b31aae2
Compare
brettz9
commented
Dec 22, 2019
@CacheControl : Thank you so much for doing this! I think if we can get this done, it will be well appreciated by many. I have a few implementation changes I intend to ask for inline shortly, but I also hope others may take a look and at least check that the changes to the tests are reasonable per their expectations.
While it looks like the comparison is done with wrap: true instead (see https://github.com/cburgmer/json-path-comparison/blob/master/implementations/JavaScript_jsonpath-plus/index.js#L23 ), I wonder if looking at the comparisons can still help get an idea of what should be expected if the other implementations offered wrap: false as an option? I'd also suggest taking a look at https://github.com/cburgmer/json-path-comparison/blob/master/OPEN_QUESTIONS.md and seeing if that analysis affects your approach.
@brettz9
brettz9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very good to have, and just want to make such we minimize performance impact. I think we're already going to need a breaking change; though this might be considered a mere fix, as it aligns with other implementations and perhaps expected behavior, others may have been relying on its quirks, so I think we'll need to bump to a new major version.
Thanks very much again for all your work on this!
brettz9
commented
Jan 13, 2020
This is great to have! Thanks so much for your work on this!
I'm bumping to a new major version as the new changes may be unexpected for some.
Due to being on a slow connection where I am, I have not been able to download the dependencies to run the json path comparison suite on my own (nor the performance suite), but it looks good, and as mentioned, with a major version bump, it should give a chance for others to adjust or report back if there are concerns.
I've also added testing coverage, and some further tests. I plan to make a new release shortly, after seeing about getting some further coverage.
brettz9
commented
Jan 14, 2020
Released as part of v3.0.0 along with a few other fixes discovered in the process of getting to 100% coverage.
Despite adding new tests, some of even these new tests may need to be revised to harmonize with cross-implementation behavior (e.g., as with the scalar differences discussed), but at least now we are specifying the current behavior more fully in our tests so upon refactoring, we can detect regressions as well as more easily specify any new breaking changes in behavior by simply refactoring existing tests.
jorchg
commented
Jan 17, 2020
@brettz9 Will you publish v3.0.0 to npm? Thank you for the great job :D
CacheControl
commented
Jan 25, 2020
@brettz9 if you're hoping to squeeze a few more breaking changes into this release, would you mind publishing a release candidate of 3.0? that way I can expedite the bug fix on the json-rules-engine side of things. thank you!
brettz9
commented
Jan 25, 2020
Sorry, @CacheControl , I thought I had published it! Done! (Probably will be future breaking changes, at least for changing TypeScript if someone can help with #113 )
brettz9
commented
Jan 25, 2020
And sorry @jorchg , I guess I had thought it was already released or hadn't seen your message for some reason. But I see it on npm now.
jorchg
commented
Feb 15, 2020
And sorry @jorchg , I guess I had thought it was already released or hadn't seen your message for some reason. But I see it on npm now.
So cool @brettz9. No problem at all. Thank you for your effort!
And thank you also to @CacheControl :)
Uh oh!
There was an error while loading. Please reload this page.
Fixes #98 - consistently detect and return arrays vs scalars.
The following paths return whatever is found at the specified location whether that is a scalar, object, or array:
The following paths will always result in an array, because they may generate multiple results depending on the dataset:
@brettz9 would you mind reviewing this please?