Bug 1449038 - Add a simple weighted search to KeywordService

[spadgett]

https://trello.com/c/2fFSJMX1
https://bugzilla.redhat.com/show_bug.cgi?id=1449038

[spadgett]

/assign @benjaminapetersen

PTAL

[benjaminapetersen]

reviewing

[benjaminapetersen]

Minor things, mostly wondering if we want it to handle both w & w/o weights.

[benjaminapetersen]

Nit: can we use _.reduce( ) or _.flatten(_.map()) vs manually creating [ ] and _.each() to .push()?

[spadgett]

Can you give some example code?

[benjaminapetersen]

Outermost:

var weightedSearch = function(objects, filterFields, keywords) {
  if(_.isEmpty(keywords)) {
    return [];
  }
  
  var results = _.reduce(objects, function(result, object) {

    // do scoring work .....
    
    if (score > 0) {
      results.push({
        object: object,
        score: score
      });
    }
    return result;
  }, []);

  // Sort first by score, then by display name for items that have the same score.
  var orderedResult = _.orderBy(results, ['score', displayName], ['desc', 'asc']);
  return _.map(orderedResult, 'object');
  
}

Could possibly do the score and matchesKeyword similarly, but def isn't a huge deal.

[spadgett]

I guess I'm not clear on the benefit.

Since _.reduce can change result to anything, you have to read the code carefully to see that it's just appending values to an array. I feel like _.each / push is easier to understand.

[benjaminapetersen]

Right, I prob should not have suggested reduce over like _.compact(_.map(objects)), its too general purpose.

Its prob just the imperative vs declarative discussion... I'd rather not see the "noise" of manually making the new array(s) when we have a number of good functions that do that for us.

That said, is def a (subjective) nit, and not a huge issue 😄

[benjaminapetersen]

big pts for tests! much easier to review & figure out what the new stuff is doing.

[benjaminapetersen]

So is this the kind of configuration we might want to do once up front, via $provider in app.js .cofig(), or would we want to custom config weights & paths for every use?

[benjaminapetersen]

ie, config some functions & cache them, so we don't have copy / paste repeats of the setup in multiple places. I don't know how many times we will use this search feature.

[spadgett]

It will vary by the kind of resource and the context, so I think this has to be up to the caller.

[benjaminapetersen]

Maybe unrelated, but wonder if .generateKeywords should take either 'string of multiple' or ['array','of','multiple'].

[benjaminapetersen]

Should we have both a search and a weightedSearch? or could weightedSearch be used w/o weights, just with keyword and objects (perhaps if paths was last & optional)?

[spadgett]

filterForKeywords is basically weightedSearch without the weights

[benjaminapetersen]

I suppose naming symmetry. Prob not big deal.

[benjaminapetersen]

/lgtm

I think all my questions answered, def looks good!