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(jsonpath): tests and code for #646 and #651 #655

Closed
wants to merge 3 commits into from

Conversation

AlexBHarley
Copy link
Contributor

@AlexBHarley AlexBHarley commented Apr 19, 2017

Courtesy of @honorcode, solves #646 and #651

Pretty happy with this, @yasserf do you want to give it a second pair of eyes.

// see setValue fnc above for special handling of array item parsing vs numeric obj member name
// e.g. 'object.1' parsing. this allows for support of parsing and differentiating object
// member names that are also numeric values
let str = this._path.replace(/\s/g, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

this is super expensive, doing three inline regexs and not even compiling them all upfront.

@yasserf
Copy link
Contributor

yasserf commented Apr 19, 2017

This is a pretty expensive solution.. I would need to look into this more first but I think it can possibly be done in a more performant manner.

@yasserf
Copy link
Contributor

yasserf commented Apr 19, 2017

I'm guessing this is also a backwards compatability change to how people used the . vs []

This is certainly correctly behaviour, just a bit concerned about impact it may cause merging it in its current state.

@AlexBHarley
Copy link
Contributor Author

So discussed this with @yasserf a bit more. We don't want to do 3 regexes here

let str = this._path.replace(/\s/g, '')
str = str.replace(/\[(.*?)\]/g, '=$1')
const parts = str.split(SPLIT_REG_EXP)

The first two should be compiled like the third one, but ideally we'd still only want to do one.

A couple of solutions discussed to avoid the replace(regex, '=$1).

  • We can store some metadata somewhere about token when surrounded by [ and ], this way we know it's an array and not a property
  • Seemingly better solution:
    • when we find a token surrounded by [ and ], put it into the token array explicitly as a number (Number(token))
    • otherwise, put it in as a string (String(token))

This way inside the setValue function we can see by the type of the token, whether we should be operating on an array or object.

@yasserf
Copy link
Contributor

yasserf commented May 24, 2017

Closing this as its pretty expensive and needs to be addressed in a different manner.

@yasserf yasserf closed this May 24, 2017
@yasserf yasserf deleted the fix/json-path-fixes branch June 25, 2017 18:00
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