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

Path resolver #241

Merged
merged 6 commits into from
Dec 19, 2020
Merged

Path resolver #241

merged 6 commits into from
Dec 19, 2020

Conversation

CacheControl
Copy link
Owner

@CacheControl CacheControl commented Dec 13, 2020

adds path resolver engine option. This allows path syntax other than the json-path default. It also replaces the current selectn built-in fallback with a long term supported solution via the pathResolver callback. Fixes #210 and #187.

@CacheControl CacheControl changed the base branch from master to next-major December 13, 2020 22:01
@naveen-shetty
Copy link

Woohh!! I like this change. I was thinking of something like this last week.

Not that I was looking to use something other than json-path but to overcome this json-path/JsonPath#272 limitation of JSON Path as it is not part of the json-path spec.

The data is my use case looks like something like this. (Yes, ideally this should be a key value pair :()

So, if I want to extract a language name for a given language code, json-path cannot really do that out of the box as it always returns an array of values while dealing with arrays.

var langs = [
  { 
    langucode: 'en',
    name: 'english' 
  },
  { 
   code: 'es',
    name: 'spanish' 
  }, 
  ....
  .....
]

Still not sure if I am going to use it but It does open up the possibility of some custom parsing.

return factValuePromise
.then(factValue => {
if (isObjectLike(factValue)) {
const pathValue = this.pathResolver(factValue, path)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it makes sense pass to params as well ? Just in case, some custom pathResolver need details from params.

Suggested change
const pathValue = this.pathResolver(factValue, path)
const pathValue = this.pathResolver(factValue, path, params)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! That's an interesting idea and I see where you're coming from - this could allow a user to configure their path resolve library differently on a condition by condition basis. 🤔

My immediate thinking is that we don't want to mix the concept of fact parameters with path resolver parameters - as things stand right now there would be unintended consequences to things like fact caching, which uses params for fact idempotency. That said, we could easily avoid that issue with a pathParams property.

For now, I think I'd like to see if this request actually materializes in the wild before I add it, and then it can be introduced as a minor version. But cool idea.

@CacheControl CacheControl merged commit f6e27a0 into next-major Dec 19, 2020
@CacheControl CacheControl deleted the path-resolver branch December 19, 2020 14:16
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.

2 participants