RFR: 2170: Warn on trailing period in PR titles [v3]

[Erik Joelsson]

On Tue, 30 Apr 2024 22:29:04 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> This patch is trying to add a new jcheck called "issuestitle". This jcheck would check if the issue's title has a trailing period or leading lowercase letter. And we would like to configure "issuestitle" jcheck as warning in the repos since in some cases, trailing periods or leading lowercase letter are valid.
>> 
>> When implementing this, I found that although we can configure a jcheck as warning in the jcheck configuration file(.jcheck/conf), we fail to differentiate between jchecks as error and jchecks as warning. When jchecks fail as warnings, they cause the local jcheck to exit with a status code of 1. Also, they will be integration blockers when Skara bots evaluate pull requests.  Therefore, in this patch, I also make Skara CLI and Skara bots be able to handle jcheck warnings properly.
>
> Zhao Song has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix test

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java line 58:

> 56: 
> 57:     private void addMessage(Check check, String message, Severity severity, boolean readyForReviewWhenFailedAsError) {
> 58:         if (severity.equals(Severity.ERROR)) {

With enums it's ok and even encouraged to use `==` for comparison.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java line 62:

> 60:             if (this.readyForReview) {
> 61:                 this.readyForReview = readyForReviewWhenFailedAsError;
> 62:             }

This part is rather confusing to try to understand. Maybe we need a separate method for updating the `readyForReview` field, and remove the new parameter `readyForReviewWhenFailedAsError` from this method, to make this interaction clearer? 


private void setNotReadyForReviewOnError(Severity severity) {
    if (severity == ERROR) {
        readyForReview = false;
    }
}


This method would be called from the places that used to do `readyForReview = false`.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java line 76:

> 74:     }
> 75: 
> 76:     List<String> hiddenMessages() {

Maybe rename this to make it clear that it's about errors only, something like `hiddenErrorMessages()`.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java line 295:

> 293:         List<String> messages = new ArrayList<>();
> 294:         if (!issue.issuesWithTrailingPeriod().isEmpty()) {
> 295:             messages.add("Found trailing period in " + String.join(" ,", issue.issuesWithTrailingPeriod()));

Shouldn't the join be comma followed with space? Maybe specify that this is in the issue title?
Suggestion:

            messages.add("Found trailing period in issue title for " + String.join(", ", issue.issuesWithTrailingPeriod()));

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java line 298:

> 296:         }
> 297:         if (!issue.issuesWithLeadingLowerCaseLetter().isEmpty()) {
> 298:             messages.add("Found leading lowercase letter in " + String.join(" ,", issue.issuesWithLeadingLowerCaseLetter()));

Suggestion:

            messages.add("Found leading lowercase letter in issue title for " + String.join(" ,", issue.issuesWithLeadingLowerCaseLetter()));

cli/src/main/java/org/openjdk/skara/cli/JCheckCLIVisitor.java line 321:

> 319:             if (!i.issuesWithLeadingLowerCaseLetter().isEmpty()) {
> 320:                 println(i, "Found leading lowercase letter in " + String.join(" ,", i.issuesWithLeadingLowerCaseLetter()));
> 321:             }

Same comment here about wording and joining as above.

jcheck/src/main/java/org/openjdk/skara/jcheck/IssuesTitleCheck.java line 71:

> 69:     @Override
> 70:     public String description() {
> 71:         return "Issue's title shouldn't have trailing period";

Suggestion:

        return "Issue's title should be properly formatted";

-------------

PR Review Comment: https://git.openjdk.org/skara/pull/1638#discussion_r1591483750
PR Review Comment: https://git.openjdk.org/skara/pull/1638#discussion_r1591498596
PR Review Comment: https://git.openjdk.org/skara/pull/1638#discussion_r1591487824
PR Review Comment: https://git.openjdk.org/skara/pull/1638#discussion_r1591509427
PR Review Comment: https://git.openjdk.org/skara/pull/1638#discussion_r1591511027
PR Review Comment: https://git.openjdk.org/skara/pull/1638#discussion_r1591513077
PR Review Comment: https://git.openjdk.org/skara/pull/1638#discussion_r1591514159