-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
do not throw if value is null #12
Conversation
Sorry, totally forgot about this one... I've changed my mind, I no longer think it makes sense to throw on accessor. Could you fix the merge conflict and only handle the other cases? |
ping @stevemao |
Hm... I was waiting for #17 to be merged first... Since I think this one is a bit tricker... I'll work on this if you think its better :) |
What do you think @SamVerschueren? |
Sorry, think I got lost somewhere in the conversation :). Where do you want my opinion on? To merge #17 first and after that this PR? |
They all need to be merged. Just need +1s on these PRs |
@@ -11,7 +11,11 @@ module.exports.get = function (obj, path) { | |||
for (var i = 0; i < pathArr.length; i++) { | |||
obj = obj[pathArr[i]]; | |||
|
|||
if (obj === undefined) { | |||
if (obj === undefined || obj === null) { | |||
if (i !== pathArr.length - 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment on why this is here? I'm a bit unsure what it's doing.
@sindresorhus I have added some comments. |
LGTM |
Sorry guys... I screwed up with git. Can you please re-push the latest commits to master branch? Or is there a way for me to restore them (I don't have those commits in my local repo)? Sorry!! |
No worries. Fixed. Btw, you can always use |
Thanks @stevemao. Glad we could finally get this merged :) |
Fixes #11.