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.

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

Add fuzzy matching (issue #101) #191

Closed
wants to merge 2 commits into from

Conversation

chris48s
Copy link
Contributor

See issue #101 for further info/notes on this feature.
Submitted for approval, but happy to discuss or do further work on this if required.
Fixes #101

@chris48s chris48s force-pushed the fuzzymatch branch 4 times, most recently from 94a1c8a to 3464f21 Compare May 23, 2015 13:15
@chris48s
Copy link
Contributor Author

Hi.
I realise it has been some time since I followed up on this, but I recently submitted a minor patch to update djorm-ext-pgtrgm for Django 1.9 compatibility ( jleivaizq/djorm-ext-pgtrgm#19 ) so it seemed like a good time to revisit this.

I've updated this PR so that /areas has a param ?fuzzy=true as suggested by @dracos in the discussion around issue #101 instead of using its own endpoint. I've also removed the code to pull this package in as a git submodule given this is not your preferred approach.
As you'll note from jleivaizq/djorm-ext-pgtrgm#19 I have raised the issue of updating the package on pypi with the original author but got no response. To review the issue of how best to pull in this dependency using pip, it occurs to me that pip can install the package directly from GitHub using the command

pip install git+git://github.com/jleivaizq/djorm-ext-pgtrgm.git

or

pip install git+git://github.com/jleivaizq/djorm-ext-pgtrgm.git@6a5eefd

(if you want to 'lock' the dependency to a particular commit). Does this sound like a reasonable solution to this issue?

You'll note that the Travis CI build fails on this commit with the error 'ImportError: No module named djorm_pgtrgm' which is unsurprising given the above. I assume that this is not something I am able to address, given the dependencies for the test build are pulled in from

/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages

Finally, if there are any other issues relating to this feature (see issue #101 for previous discussions) that you would like me to do anything further on, let me know and I will follow up.

Thanks

@dracos
Copy link
Member

dracos commented Nov 18, 2015

"I assume that this [Travis failure] is not something I am able to address" - Travis (well, tox) only installs what setup.py is telling it, which is what you'll get when you install mapit from pypi, so it does need to be fixed before it can be merged :) I'm afraid you can't install source control sources in setup.py, only actual packages, so (though see below) the only solution in the absence of upstream update would be for someone to create their own package on pypi containing this code and we can then reference that.

However, as we only support Django 1.8+ now, adding such a lookup is only a few lines, I don't think we need the third party module at all for that. This could be done similarly to the usage example I gave at https://github.com/django/django/pull/4815/files#diff-7c3d79c64cab66e031bd47e81345207fR29 with the lookup/functions given in that PR, which would be straightforward to include. The 'CREATE EXTENSION' (and I guess index) could be done with a migration, though I'm not sure if it shouldn't be left as a manual step in case someone doesn't have the extension installed. Does including the required lookups directly here sound okay to you?

@chris48s
Copy link
Contributor Author

That makes things easier then - thanks

@dracos dracos force-pushed the master branch 3 times, most recently from f8a2f78 to bcb87c5 Compare June 7, 2016 17:10
@chris48s chris48s closed this Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable fuzzy string searching/matching for areas
2 participants