Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fuzziness #41

Closed
wants to merge 13 commits into from
Closed

Fuzziness #41

wants to merge 13 commits into from

Conversation

hkrishna
Copy link
Contributor

This implements fuzzy suggest - you can now actually have a typo in your search term and the right result will show up. The fuzziness factor is set to AUTO

because it generates an edit distance based on the length of the term. For lengths:
0..1 - must match exactly
1..4 - one edit allowed
">4"- two edits allowed

@missinglink
Copy link
Member

hey @hkrishna I wasn't sure if you wanted to merge this one yet so I just pulled your 'getting bbox test to pass' commit in to master to get the pretty green travis badge bb04a01

@hkrishna
Copy link
Contributor Author

hkrishna commented Dec 2, 2014

oh yeah, this is good to go. I left it open for you to review/merge it.

@missinglink
Copy link
Member

looks good, I'll pull and test locally, two comments:

  • /suggest/nearby doesn't seem to be updated in this PR, is that intentional?

it's a bit annoying that we have to maintain 2 suggesters but I still think /suggest is best for multi-country/planet imports and /suggest/nearby is better for smaller, single-country or single-city imports.

  • the controller/suggest.js controller is becoming large and unruly.

I am struggling to figure out what's going on in there, is there some way we can re-factor out the individual components so they can be re-used and tested independently?

@hkrishna
Copy link
Contributor Author

hkrishna commented Dec 2, 2014

fair point. This is why I like code reviews 😄

/suggest/nearby is left untouched intentionally for now. Was gonna refactor the code to pull our things that can be reused and use it in /suggest/nearby

I agree that controller/suggest.js is becoming large and crazy. I do have a refactor in mind #techdebt

We could close this PR without merging and I can work on a refactor?

@hkrishna
Merge branch 'master' into fuzzy
c4aa04c 
Conflicts:
	test/unit/controller/suggest.js
	test/unit/helper/geojsonify.js
@hkrishna
moving the query mixing logic out of the controllers and into helper/…
e405c9f 
…queryMizer.json, deleting controller/suggest_nearby.js and test/unit/controller/suggest_naerby.js (because suggest_nearby is like suggest with a different query-mix. disabling a test in suggest.js for the moment
@hkrishna
functional failure test fixed
a1d4d79
@hkrishna
lint: line too long
47f8709
@hkrishna
test queryMixer.json
e9f7e6c
@hkrishna hkrishna mentioned this pull request Dec 8, 2014
Closed
@hkrishna
Copy link
Contributor Author

closing this in favor of #59

@hkrishna hkrishna closed this Dec 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@hkrishna @missinglink