mcollina / bloomrun

A js pattern matcher
MIT License
120 stars 10 forks source link

Deeper matches should win #23

Closed mcdonnelldean closed 8 years ago

mcdonnelldean commented 8 years ago

@mcollina The following test is failing, with the smaller patter being matched:

test('deeper matches should win - #', function (t) {
  t.plan(1)

  var instance = bloomrun()

  instance.add({ role: 'auth', cmd: 'login' }, 42)
  instance.add({ role: 'auth', cmd: 'login', retry: 'true' }, 24)

  t.equal(instance.lookup({role: 'auth', cmd: 'login', retry: 'true'}), 24)
})

If you reverse the patterns the test passes. This is because the code in deep match is only validating the params on the pattern. If a smaller pattern fully matches a larger pattern and it was inserted first it wins because our check if for full match on pattern. See https://github.com/mcollina/bloomrun/blob/master/lib/deepMatch.js#L16-L18

Any suggestions? The only way I can see right now is to sort the matching buckets.data by length, that way we are always looking at the larger matches first. This of course would require a major bump because we are going against insertion order.

Any other suggestions?

mcollina commented 8 years ago

I'd say put an option on the constructor, and make it non default to havr the resorting on add.

What do you think? Il giorno mer 18 nov 2015 alle 11:29 Dean McDonnell < notifications@github.com> ha scritto:

@mcollina https://github.com/mcollina The following test is failing, with the smaller patter being matched:

test('deeper matches should win - #', function (t) { t.plan(1)

var instance = bloomrun()

instance.add({ role: 'auth', cmd: 'login' }, 42) instance.add({ role: 'auth', cmd: 'login', retry: 'true' }, 24)

t.equal(instance.lookup({role: 'auth', cmd: 'login', retry: 'true'}), 24) })

If you reverse the patterns the test passes. This is because the code in deep match is only validating the params on the pattern. If a smaller pattern fully matches a larger pattern and it was inserted first it wins because our check if for full match on pattern. See https://github.com/mcollina/bloomrun/blob/master/lib/deepMatch.js#L16-L18

Any suggestions? The only way I can see right now is to sort the matching buckets.data by length, that way we are always looking at the larger matches first. This of course would require a major bump because we are going against insertion order.

Any other suggestions?

— Reply to this email directly or view it on GitHub https://github.com/mcollina/bloomrun/issues/23.

mcdonnelldean commented 8 years ago

Yup, I'm happy with that. I'll get it done tonight.