Replace defaultTo with undefined check in pathOr #2888
Conversation
There was a problem hiding this comment.
Thanks for submitting this @Tworley96! It looks good!
I made a couple of comments, I think we should iron out what the intended functionality is on this.
| @@ -23,6 +23,7 @@ import path from './path'; | |||
| * R.pathOr('N/A', ['a', 'b'], {c: {b: 2}}); //=> "N/A" | |||
| */ | |||
| var pathOr = _curry3(function pathOr(d, p, obj) { | |||
| return defaultTo(d, path(p, obj)); | |||
| var val = path(p, obj) | |||
| return val === undefined ? d : val | |||
| }); | |||
There was a problem hiding this comment.
Can you please clean up the linting errors in this file. You can test it by running npm run lint
|
I think what we have documented is what we probably want to support, even if that breaks backward compatibility: That this is different from |
There was a problem hiding this comment.
I cleaned up the linting errors but had to redesign some things somewhat significantly to meet the new requirements. I did not fully clean up the code because I was not sure if this is the correct path, if it is I will make the code more organized. If paths is to continue to be used as the base for prop,path, propOr, and pathOr it needs to throw an error as simply returning undefined does not give propOr an pathOr enough information to determine whether or not to default. Similarly with using nth in paths. nth doesn't differentiate between returning undefined in case of finding it at the index in the list versus the index exceding the bounds of the list. I'd just like to double check that this is the desired behavior because it doesn't seem to necessarily 'fit' with the behavior of other library functions (see nth). I could also see the logic in saying { a: undefined } is not a supported case because undefined is supposed to be a seminal value.
On a somewhat unrelated note, I found another behavior that I presumed was a bug so I went ahead and accounted for in my changes, but I can change that if it is intended behavior. https://ramdajs.com/repl/?v=0.26.1#?R.path%28%5B1%5D%2C%20%27Alice%27%29
| @@ -54,8 +54,8 @@ describe('path', function() { | |||
| eq(R.path(['a', 'b'], {a: null}), undefined); | |||
| }); | |||
|
|
|||
| it('works with falsy items', function() { | |||
| eq(R.path(['toString'], false), Boolean.prototype.toString); | |||
| it('returns undefined for non-object items', function() { | |||
There was a problem hiding this comment.
toString in false errors, so this test no longer makes sense
|
It's been awhile but I believe that this has fixed all of the stated issues. If not let me know. |
|
Yes, it does. Sorry this got dropped. Thanks for the contribution! 🌿 |
|
@Tworley96 @CrossEye I've reverted this merge because it breaks our test suite. Can we get an update so we can add this to the codebase? |
|
I've updated the code so that the test suite passes in #2995. The code that was reverted will undo some of the optimizations in #2962. Personally, I think some of these changes might not be the best and there might be some more discussion to be had about how these functions handle null vs undefined. |
Weird, how did I miss that!? |
No description provided.