Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

- Fix: wrap: false returning inconsistent data types #111

Merged

Conversation

CacheControl
Copy link

@CacheControl CacheControl commented Dec 21, 2019

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:

  • "store.book[0]" - specific book
  • "store.bicycle.red" - single property of a single object

The following paths will always result in an array, because they may generate multiple results depending on the dataset:

  • $.store.book[0][category,author] - property or index lists return multiple values
  • $..book - ".." will recurse through the store object
  • $.store.book[1:2] - indicates a range within the array
  • $.store.book[*] - wild card indicates multiple results
  • $.store.book[?(@.isbn)] - filtering

@brettz9 would you mind reviewing this please?

@CacheControl CacheControl force-pushed the wrap-false-consistent-data-type branch from 485bd8d to 6762a51 Compare December 22, 2019 01:21
@CacheControl CacheControl force-pushed the wrap-false-consistent-data-type branch from 6762a51 to b31aae2 Compare December 22, 2019 01:29
@brettz9
Copy link
Collaborator

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.

Copy link
Collaborator

@brettz9 brettz9 left a 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 brettz9 merged commit 7c36396 into JSONPath-Plus:master Jan 13, 2020
@brettz9
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

jorchg commented Jan 17, 2020

@brettz9 Will you publish v3.0.0 to npm? Thank you for the great job :D

@CacheControl
Copy link
Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrap: false does not correctly return some cases
3 participants