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 indexing into array with deep propery #385

Merged
merged 2 commits into from
Mar 3, 2015
Merged

Conversation

matthew-n
Copy link
Contributor

  • added unit test to expect with code from documentation
  • added default to res in _getPathValue

* added unit test to expect with code from documentation
* added default to res in _getPathValue
@keithamus
Copy link
Member

@eldritch-fossicker thanks very much for the PR, but I must admit I can't really tell what this is solving or doing. Could you please elaborate on your explanation with what this aims to fix; perhaps with examples of currently broken code that will be fixed by this PR? Thanks

@matthew-n
Copy link
Contributor Author

certainly the I had a test that kept failing which was very similar to:

expect(deepObj).to.have.property('teas')
  .that.is.an('array')
  .with.deep.property('[2]')
    .that.deep.equals({ tea: 'konacha' });

(from the chain-able portion of the property documentation) Trying to walk into the array by index always failed. So, I added the snipped from the documentation to the unit tests. Then I solved the problem by changing _getPathValue to return a value other than undefined to info.parent when path.length = 1

@keithamus
Copy link
Member

I guess what is not making sense to me is the fix. It seems to be relying on a side-effect added to _getPathValue rather than explicitly tackling the problem. It feel as though the current fix is somewhere non-obvious and may well trip people up when refactoring. To me, it seems better to change L35 from:

parent: _getPathValue(parsed, obj, parsed.length - 1),

to:

parent: parsed.length > 1 ? _getPathValue(parsed, obj, parsed.length - 1) : obj,

As this is much more explicit as an indicator for what is going on. What are your thoughts @eldritch-fossicker?

@matthew-n
Copy link
Contributor Author

@keithamus that will certainly fix the problem. My first conclusion when I looked at the function was that walking path to an index of 0 should return the object itself; therefore, the function should fixed. I think a conditional return or comment would be in order. But, I'm just concerned with fixing the error whichever solution.

@keithamus
Copy link
Member

I see where you're coming from now @eldritch-fossicker. Ultimately it's neither here nor there - but the original patch seems slightly less explicit. On your point about if it were called with an index of 0 - I'd suggest that it shouldn't be called at all if that is the case.

If you could amend the patch to my reflect my comments above I'd be very happy at merging this though 😄

@keithamus
Copy link
Member

🎉

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