From 5da1a5a6970bd1b36b57e47711e3fe1d457e2e3c Mon Sep 17 00:00:00 2001 From: Robin Westberg Date: Sun, 6 Sep 2020 08:50:17 +0000 Subject: [PATCH] Improve bot message formatting Reviewed-by: ehelin --- .../bots/notify/PullRequestWorkItem.java | 37 ++++++++++--------- .../comment/CommitCommentNotifierTests.java | 5 +-- .../skara/bots/pr/IntegrateCommand.java | 4 +- .../skara/bots/pr/LabelerWorkItem.java | 15 +++++--- .../openjdk/skara/bots/pr/SponsorCommand.java | 4 +- .../org/openjdk/skara/bots/pr/LabelTests.java | 2 +- .../openjdk/skara/bots/pr/LabelerTests.java | 2 +- 7 files changed, 38 insertions(+), 31 deletions(-) diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/PullRequestWorkItem.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/PullRequestWorkItem.java index 98747930c..742ff9516 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/PullRequestWorkItem.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/PullRequestWorkItem.java @@ -24,6 +24,7 @@ import org.openjdk.skara.bot.WorkItem; import org.openjdk.skara.forge.PullRequest; +import org.openjdk.skara.issuetracker.Comment; import org.openjdk.skara.json.*; import org.openjdk.skara.storage.StorageBuilder; import org.openjdk.skara.vcs.Hash; @@ -32,7 +33,7 @@ import java.nio.file.Path; import java.util.*; import java.util.function.Consumer; -import java.util.regex.Pattern; +import java.util.regex.*; import java.util.stream.*; public class PullRequestWorkItem implements WorkItem { @@ -50,21 +51,21 @@ public class PullRequestWorkItem implements WorkItem { this.integratorId = integratorId; } - private Hash resultingCommitHashFor(PullRequest pr) { - if (pr.labels().contains("integrated")) { - for (var comment : pr.comments()) { - if (comment.author().id().equals(integratorId)) { - for (var line : comment.body().split("\n")) { - if (line.startsWith("Pushed as commit")) { - var parts = line.split(" "); - var hash = parts[parts.length - 1].replace(".", ""); - return new Hash(hash); - } - } - } - } - } - return null; + private final static Pattern pushedPattern = Pattern.compile("Pushed as commit ([a-f0-9]{40})\\."); + + private Hash resultingCommitHash() { + if (pr.labels().contains("integrated")) { + return pr.comments().stream() + .filter(comment -> comment.author().id().equals(integratorId)) + .map(Comment::body) + .map(pushedPattern::matcher) + .filter(Matcher::find) + .map(m -> m.group(1)) + .map(Hash::new) + .findAny() + .orElse(null); + } + return null; } private Set deserializePrState(String current) { @@ -179,7 +180,7 @@ public Collection run(Path scratchPath) { .materialize(historyPath); var issues = parseIssues(); - var commit = resultingCommitHashFor(pr); + var commit = resultingCommitHash(); var state = new PullRequestState(pr, issues, commit); var stored = storage.current(); if (stored.contains(state)) { @@ -193,7 +194,7 @@ public Collection run(Path scratchPath) { .findAny(); // The stored entry could be old and be missing commit information - if so, upgrade it if (storedState.isPresent() && storedState.get().commitId().equals(Optional.of(Hash.zero()))) { - var hash = resultingCommitHashFor(pr); + var hash = resultingCommitHash(); storedState = Optional.of(new PullRequestState(pr, storedState.get().issueIds(), hash)); storage.put(storedState.get()); } diff --git a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/comment/CommitCommentNotifierTests.java b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/comment/CommitCommentNotifierTests.java index e1a2ae220..10e13a17a 100644 --- a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/comment/CommitCommentNotifierTests.java +++ b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/comment/CommitCommentNotifierTests.java @@ -23,12 +23,11 @@ package org.openjdk.skara.bots.notify.comment; import org.junit.jupiter.api.*; -import org.openjdk.skara.bots.notify.*; +import org.openjdk.skara.bots.notify.NotifyBot; import org.openjdk.skara.json.JSON; import org.openjdk.skara.test.*; import java.io.IOException; -import java.net.URI; import java.util.*; import java.util.regex.Pattern; @@ -76,7 +75,7 @@ void testCommitComment(TestInfo testInfo) throws IOException { var pr = credentials.createPullRequest(repo, "master", "master", "Fix an issue"); pr.setBody("I made a fix"); pr.addLabel("integrated"); - pr.addComment("Pushed as commit " + editHash.hex() + "."); + pr.addComment("More text!\n\n@user Pushed as commit " + editHash.hex() + ". Even more text.\n\nAnd some additional text."); TestBotRunner.runPeriodicItems(notifyBot); // Check commit comment diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java index 20942ba3e..d3bd1261f 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java @@ -155,7 +155,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst if (!finalRebaseMessage.isBlank()) { reply.println(rebaseMessage.toString()); } - reply.println("Pushed as commit " + localHash.hex() + ". :bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored."); + reply.println("Pushed as commit " + localHash.hex() + "."); + reply.println(); + reply.println(":bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored."); localRepo.push(localHash, pr.repository().url(), pr.targetRef()); pr.setState(PullRequest.State.CLOSED); pr.addLabel("integrated"); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java index 07476f6db..dd3c948d2 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java @@ -73,25 +73,28 @@ private void updateLabelMessage(List comments, List newLabels) if (newLabels.isEmpty()) { message.append("To determine the appropriate audience for reviewing this pull request, one or more "); message.append("labels corresponding to different subsystems will normally be applied automatically. "); - message.append("However, no automatic labelling rule matches the changes in this pull request. "); + message.append("However, no automatic labelling rule matches the changes in this pull request.\n\n"); message.append("In order to have an RFR email automatically sent to the correct mailing list, you will "); message.append("need to add one or more labels manually using the `/label add \"label\"` command. "); message.append("The following labels are valid: "); var labels = bot.labelConfiguration().allowed().stream() .sorted() .map(label -> "`" + label + "`") - .collect(Collectors.joining(", ")); + .collect(Collectors.joining(" ")); message.append(labels); message.append("."); } else { - message.append("The following labels will be automatically applied to this pull request: "); + message.append("The following label"); + if (newLabels.size() > 1) { + message.append("s"); + } + message.append(" will be automatically applied to this pull request: "); var labels = newLabels.stream() .sorted() .map(label -> "`" + label + "`") - .collect(Collectors.joining(", ")); - message.append("`"); + .collect(Collectors.joining(" ")); message.append(labels); - message.append("`. When this pull request is ready to be reviewed, an RFR email will be sent to the "); + message.append(".\n\nWhen this pull request is ready to be reviewed, an RFR email will be sent to the "); message.append("corresponding mailing list"); if (newLabels.size() > 1) { message.append("s"); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java index 8a6706639..8766d9510 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java @@ -129,7 +129,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst if (!finalRebaseMessage.isBlank()) { reply.println(rebaseMessage.toString()); } - reply.println("Pushed as commit " + localHash.hex() + ". :bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored."); + reply.println("Pushed as commit " + localHash.hex() + "."); + reply.println(); + reply.println(":bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored."); localRepo.push(localHash, pr.repository().url(), pr.targetRef()); pr.setState(PullRequest.State.CLOSED); pr.addLabel("integrated"); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelTests.java index 290385f76..5d91216e0 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelTests.java @@ -163,7 +163,7 @@ void adjustAutoApplied(TestInfo testInfo) throws IOException { // The bot should have applied one label automatically TestBotRunner.runPeriodicItems(prBot); assertEquals(Set.of("2", "rfr"), new HashSet<>(pr.labels())); - assertLastCommentContains(pr, "The following labels will be automatically applied"); + assertLastCommentContains(pr, "The following label will be automatically applied"); assertLastCommentContains(pr, "`2`"); // It will refuse to remove it diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelerTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelerTests.java index d918d2fe8..aded5f18e 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelerTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelerTests.java @@ -71,7 +71,7 @@ void simple(TestInfo testInfo) throws IOException { TestBotRunner.runPeriodicItems(labelBot); assertEquals(Set.of("rfr"), new HashSet<>(pr.labels())); assertLastCommentContains(pr, "However, no automatic labelling rule matches the changes in this pull request."); - assertLastCommentContains(pr, "The following labels are valid: `test1`, `test2`"); + assertLastCommentContains(pr, "The following labels are valid: `test1` `test2`"); var fileA = localRepoFolder.resolve("a.txt"); Files.writeString(fileA, "Hello");