Code review comment for lp:~dobey/unity-scope-click/recommends

This branch is lacking many tests.

Here's a non exhaustive list of tests that would make sense adding:
- package_list_from_json_node: single package, multiple packages, some missing field in a package
- Highlight::from_json_node: single highlight, multiple highlight, missing ci_package field in a highlight

----

Please add names to make the callback parameters more obvious, eg:
std::function<void(Packages, Packages)> callback ->
std::function<void(Packages search_results, Packages recommended)> callback

----

Please refactor the following block out of Index::search into a new method, and add tests for it:

34 + if (root.isObject() && root.isMember(Package::JsonKeys::embedded)) {
35 + auto const emb = root[Package::JsonKeys::embedded];
36 + if (emb.isObject() && emb.isMember(Package::JsonKeys::ci_package)) {
37 + auto const pkg = emb[Package::JsonKeys::ci_package];
38 + pl = click::package_list_from_json_node(pkg);
39 +
40 + if (emb.isMember(Package::JsonKeys::ci_recommends)) {
41 + auto const rec = emb[Package::JsonKeys::ci_recommends];
42 + recommends = click::package_list_from_json_node(rec);
43 + }
44 + }
45 + } else if (root.isArray()) {
46 + qDebug() << "Fell back to old array mode.";
47 + pl = click::package_list_from_json_node(root);
48 + }

Also, please check if the old Array mode is really needed, and if not let's get rid of it.

---

Why are you not using push_package in 375 and below?

371 foreach (auto p, packages) {
372 push_package(searchReply, category, installedPackages, p);
373 }
374 + foreach (auto r, recommends) {
375 + try {

If there's any reason not to use push_package, lines 375 and below should be refactored to a new method and tested.

---

Please check with design if installed packages should be shown or not on recommendations, and let's open a bug if they should be filtered.