Fix hasOwnProperty bug again and forever #81
Conversation
src/Data/StrMap.js
Outdated
| var r = {}; | ||
| for (var k in m) { | ||
| if (m.hasOwnProperty(k)) { | ||
| if ({}.hasOwnProperty.call(m, k)) { |
There was a problem hiding this comment.
Can we make a variable for {} and reuse it throughout please? {} does actually go through the process of constructing a new object every time, so if in heavily called code the overhead can be noticeable.
There was a problem hiding this comment.
var hasOwnProperty = {}.hasOwnProperty; would also cache the lookup of the method. 👌🏻
src/Data/StrMap.js
Outdated
|
|
||
| // module Data.StrMap | ||
|
|
||
| var hasOwnProperty = {}.hasOwnProperty; |
There was a problem hiding this comment.
You can just delete this line. The global object already has this binding for you.
src/Data/StrMap.js
Outdated
| var r = {}; | ||
| for (var k in m) { | ||
| if (m.hasOwnProperty(k)) { | ||
| if (hasOwnProperty(m, k)) { |
There was a problem hiding this comment.
_copy isn't being used at all. Let's delete it.
|
Looks good to me now if @michaelficarra is also happy? @paf31 any objections? I'm not really sure why previous attempts at this weren't accepted 😄 |
test/Test/Data/StrMap.purs
Outdated
| b <- arbitrary | ||
| k <- arbitrary | ||
| kIshasOwnProperty <- (&&) <$> arbitrary <*> arbitrary | ||
| k <- if kIshasOwnProperty then pure "hasOwnProperty" else arbitrary |
There was a problem hiding this comment.
Might frequency be a more idiomatic way of saying that we should occasionally use 'hasOwnProperty' as a key? https://pursuit.purescript.org/packages/purescript-quickcheck/3.1.1/docs/Test.QuickCheck.Gen#v:frequency
There was a problem hiding this comment.
Ya! Didn't know about that one, will add. 👍
michaelficarra
left a comment
michaelficarra
left a comment
There was a problem hiding this comment.
LGTM after frequency is used.
|
Looks good to me. I just wanted to avoid the approach where we mangled keys, since Thanks @rightfold! |
5 participants
Seems to have been fixed several times before, but somehow still slipped through.
Uses the same check record updates use now.