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

Multiple word searching with List.search - closes #680 #682

Closed
wants to merge 38 commits into from

Conversation

sheffieldnikki
Copy link
Contributor

Default search function now supports fast multi-word searching as well as exact matching of quoted phrases. See #680

Added documentation to List API and test units to __test__/search.test.js
(also fixes a few typos in __test__/search.test.js)

clbn and others added 30 commits November 17, 2017 16:02
Changed license date
Now LICENSE is up to date.
LICENSE and README.md files are not up to date
Fix typos in error message
@codecov-io
Copy link

codecov-io commented Apr 2, 2020

Codecov Report

Merging #682 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #682   +/-   ##
=======================================
  Coverage   94.78%   94.78%           
=======================================
  Files          19       19           
  Lines         806      806           
  Branches      190      190           
=======================================
  Hits          764      764           
  Misses         29       29           
  Partials       13       13           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec685e7...ec685e7. Read the comment docs.

@sheffieldnikki
Copy link
Contributor Author

sheffieldnikki commented Apr 5, 2020

If this multi-word search is accepted into the library, replacing the default search, is it worth having fuzzy search built-in as well? Fuzzy search is 10% of the List.min.js size, and it could easily be built as a separate file list-fuzzysearch.js (doesn't even need to be a plugin) that provides a custom search function for projects that need it?

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@dfc930e). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #682   +/-   ##
=========================================
  Coverage          ?   94.78%           
=========================================
  Files             ?       19           
  Lines             ?      806           
  Branches          ?      190           
=========================================
  Hits              ?      764           
  Misses            ?       29           
  Partials          ?       13           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfc930e...ec685e7. Read the comment docs.

Comment on lines 58 to +59
for (var k = 0, kl = list.items.length; k < kl; k++) {
search.item(list.items[k]);
}
},
item: function(item) {
item.found = false;
for (var j = 0, jl = columns.length; j < jl; j++) {
if (search.values(item.values(), columns[j])) {
item.found = true;
return;
}
}
},
values: function(values, column) {
if (values.hasOwnProperty(column)) {
text = list.utils.toString(values[column]).toLowerCase();
if ((searchString !== "") && (text.search(searchString) > -1)) {
return true;
var item = list.items[k];
Copy link

@buczek buczek Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (var item of list.items) { ?
(same with following loops)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The array iteration used in the code was tested as being the fastest across multiple browsers, and is also compatible with all browsers. for ... of ... isn't compatible with older browsers.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then its certainly better the way it is 👍

for (var i = 0, il = words.length; i < il; i++) {
var word_found = false;
for (var j = 0, jl = columns.length; j < jl; j++) {
var values = item.values(), column = columns[j];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lift var values = item.values()out of the two inner loops?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definite improvement :) Thanks

@javve
Copy link
Owner

javve commented Nov 23, 2020

Merged into #696

@javve javve closed this Nov 23, 2020
@eng1neer
Copy link

Great update! Looks like it unexpectedly (well, at least to me 🙂) changed the behavior of searches that contain URLs though: previously in version 1.5.0 I could search for "example.com" just fine, and now it seems to split it into two words and it stopped working. Any way to search for an exact match (quoted "example.com" does not work either)?

@sheffieldnikki
Copy link
Contributor Author

Great update! Looks like it unexpectedly (well, at least to me slightly_smiling_face) changed the behavior of searches that contain URLs though: previously in version 1.5.0 I could search for "example.com" just fine, and now it seems to split it into two words and it stopped working. Any way to search for an exact match (quoted "example.com" does not work either)?

Can you submit a new issue with this bug please? and example code showing it failing. This code should only split words on whitespace (space, tab, newline) characters, not on a period, so 'example.com' should work without needing exact-match quoting.

@eng1neer
Copy link

@sheffieldnick #715

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.

9 participants