Closed richardpoole closed 9 years ago
Fancy adding a test to prevent this from happening again?
Not sure how I'd write a test for this since the method returns the correct value. I guess you could compare execution time between the first and subsequent calls, but that seems fragile.
I guess that its output isn't covered by any of the existing tests is the problem.
I think it's covered here: https://github.com/olivernn/lunr.js/blob/master/test/vector_test.js#L3
Ah yes, but not the caching. Oh well! I'll go back to sleep now :sleeping:
I saw that yesterday as well, but also noticed another thing: The insert
method doesn't invalidate the cache when it adds new Node
s.
Maybe that's ok, since Vectors seem to be mostly one-use, but in cases where they might not be, the typo actually prevents stale cache issues.
Thanks @satchmorun. I've added cache invalidation to Vector.insert()
and a corresponding test.
Tpying is hrd! Good spot, wish there was a better way to prevent my fat fingered mashing of the keyboard resulting in bugs like this, any ideas?
https://www.youtube.com/watch?v=OqjF7HKSaaI
I've released version 0.5.8 with this change in, let me know if there are any issues, and thanks again for the pr.
Haha, easily done. Caching code isn't exactly easy to test. :)
We could try something like this (not tested):
var cached = function (key, func) {
return function () {
if (this[key]) return this[key]
return this[key] = func.apply(this, arguments);
}
}
lunr.Vector.prototype.magnitude = cached('_magnitude', function () {
var node = this.list,
sumOfSquares = 0,
val
while (node) {
val = node.val
sumOfSquares += val * val
node = node.next
}
return Math.sqrt(sumOfSquares)
})
However it doesn't prevent typos in the invalidation code (e.g. this._magniture = undefined
) and it makes the code harder to read. Personally, I think the risk of a typo in the caching code is the lesser of two evils.
A typo in
Vector.magnitude()
was preventing it from returning the cached value