More on adjunct highlights

[ahgittin]

Applies #813 to a policy (ServiceRestarter) to see how it works. As reported there it works good, REST and persistence acting as expected.

The API for policy authors was a little awkward, and there were some PR review comments in #813. This addresses both of these.

I will be adding highlight info to more policies and adjuncts. If this is reviewed in time I'll do it in a different branch, else I'll add them here for simplicity.

[ahgittin]

See 5ae9c41#diff-0724222c7ef44a5337e1e8f44d9d5faeR114 for illustration of what I mean by an easier adjunct java author API (just some convenience methods to allow DRY).

[ahgittin]

Now has highlights on most things.

[Graeme-Miller]

lgtm, only minor comments

[Graeme-Miller]

This method would benefit from comments to explain what it is doing, with multiple loops and nested ifs it isn't easy to tell at a glance.

[Graeme-Miller]

Should be calling setHighlight, rather than putting directly into the map. This would allow these methods to remain uncoupled from how we store the highlights. Same for the below highlight methods.

[ahgittin]

agree

[Graeme-Miller]

do we have a style guide for brooklyn? I have never seen someone put a for loop on the same line as an if. Seems to break with our coding conventions.

[m4rkmckenna]

Agree for the sake of readability please add braces and a line break

[ahgittin]

clearly bad style, no argument! i added the guard when debugging, forgot to format it nicely...

[Graeme-Miller]

lines 620 -> 639 are confusing and it is difficult to extract what the code is supposed to do. I had a go at what I think is a simpler solution:

        if(!myList.isEmpty() && !(myList.size() == 1 && myList.iterator().next().equals(getEntity())) {
            msg.append(" on ");
            List<String> stringList = myList.stream().map((Sensor s) -> {
                if (s.equals(getEntity())) {
                    return "self";
                } else if (s instanceof Sensor) {
                    return ((Sensor<?>) s).getName();
                } else {
                    return s.toString();
                }
            }).collect(Collectors.toList());
            msg.append(String.join(",", stringList));
        }

[grkvlt]

Interesting @Graeme-Miller - Not sure that you need the second clause in your if, as the body handles the self output the same as the original code. I'd prefer the Collectors.joining(", ") collector, to get a String out directly... Also, I'm not sure you should try and chest the lambda type inference like that, just go s -> { /* stuff */ } and it'll be fine. Redoing the whole method with lambdas gave me this, which is almost exactly the same length ;)

    protected <T,U> void highlightTriggers(Iterable<? extends Sensor<? extends T>> sensors, Iterable<U> sources) {
        StringBuilder msg = new StringBuilder("Listening for ");
        if (Iterables.isEmpty(sensors)) {
            msg.append("<nothing>");
        } else {
            String sensorsText = StreamSupport.stream(sensors.spliterator(), false)
                    .filter(s -> s != null)
                    .map(s -> {
                        if (s instanceof Sensor) {
                            return ((Sensor<?>) s).getName();
                        } else {
                            return s.toString();
                        }
                    })
                    .collect(Collectors.joining(", "));
            msg.append(sensorsText);
        }
        if (!Iterables.isEmpty(sources)) {
            String sourcesText = StreamSupport.stream(sources.spliterator(), false)
                    .filter(s -> s != null)
                    .map(s -> {
                        if (s.equals(getEntity())) {
                            return "self";
                        } else if (s instanceof Sensor) {
                            return ((Sensor<?>) s).getName();
                        } else {
                            return s.toString();
                        }
                    })
                    .collect(Collectors.joining(", "));
            msg.append(" on ")
               .append(sourcesText);
        }
        highlightTriggers(msg.toString());
    }

[Graeme-Miller]

@grkvlt very interesting, didn't know about Collectors.joining(", "), that's very handy.

re. the second clause in the if. My understanding of Alex's code is that, if there is only one entry, and that entry is self, then we shouldn't add "on...". The second clause in the if is to guard against that. Maybe Alex can clarify?

[ahgittin]

great suggestions -- so used to using lambas in JS but not in java :)

using choice operator to make more concise, going with:

        if (sensors==null || Iterables.isEmpty(sensors)) {
            msg.append("<nothing>");
        } else {
            String sensorsText = StreamSupport.stream(sensors.spliterator(), false)
                    .filter(s -> s != null)
                    .map(s -> (s instanceof Sensor ? ((Sensor<?>) s).getName() : s.toString()))
                    .collect(Collectors.joining(", "));
            msg.append(sensorsText);
        }

        if (sources!=null && !Iterables.isEmpty(sources)) {
            String sourcesText = StreamSupport.stream(sources.spliterator(), false)
                    .filter(s -> s != null)
                    .map(s -> (s.equals(getEntity()) ? "self" : s.toString()))
                    .collect(Collectors.joining(", "));
            if (!"self".equals(sourcesText)) {
                msg.append(" on ").append(sourcesText);
            }
        }

[grkvlt]

Looks nice @ahgittin - it's taken me a while to get used to operating in this style with Java as well, but it does makes things both concise and still readable...

[ahgittin]

all addressed, thanks. if build passes then let's merge.

[ahgittin]

fixed test failure as streaming with lambdas enforces generic information (did not know that - kinda cool actually)