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

added insert function #4

Merged
merged 2 commits into from
May 8, 2016
Merged

Conversation

davidworkman9
Copy link
Contributor

Added similar functionality to object-path for inserting at an array index.

@coveralls
Copy link

coveralls commented May 2, 2016

Coverage Status

Coverage increased (+0.6%) to 97.273% when pulling 2d1d9d0 on davidworkman9:master into 0d45c87 on mariocasciaro:master.

at = ~~at;
return changeImmutable(obj, path, function(clonedObj, finalPath) {
var arr = clonedObj[finalPath];
if (!isArray(arr))
Copy link
Owner

Choose a reason for hiding this comment

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

We should force arr to be an array only if it's not set (NULL or undefined), we should throw an error instead if arr is set and it's not an Array, this will avoid silent bugs.

@mariocasciaro
Copy link
Owner

Hi Dave, thanks for the PR, it looks great.
Before I merge it, could you please take a look at the comment I left you on the code?

@coveralls
Copy link

coveralls commented May 5, 2016

Coverage Status

Coverage increased (+0.1%) to 96.847% when pulling b2369a9 on davidworkman9:master into 0d45c87 on mariocasciaro:master.

@davidworkman9
Copy link
Contributor Author

Does this address the concerns?

@mariocasciaro
Copy link
Owner

Thanks a lot for the PR, merged now

@mariocasciaro mariocasciaro merged commit 42936f9 into mariocasciaro:master May 8, 2016
@mariocasciaro
Copy link
Owner

When you have some time could you please send another PR adding something in the readme to document the new function?

@mikhuang mikhuang mentioned this pull request Aug 2, 2017
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.

None yet

3 participants