Code Review Bot

Development
#1

So as a few of you may know, I’ve been working on a Code Review Bot for quite some time.
The code is at GitHub - MeFisto94/jaime-ci-bot: A CI Bot based on Probot to aid Pull Requests on the SDK Repository.

Now I’ve hit a roadblock, namely that check_suite.(re)requested jaime-ci-bot/index.js at b506954c4f19b78c3b7f548324d79179daf6b640 · MeFisto94/jaime-ci-bot · GitHub) is not called for foreign PRs, only for local commits and local PRs.
This is done for security purposes, I guess, but github actions definitely creates a check_suite, which we’d love to hook into. Especially because a) it runs whenever a new commit is happening (or a force push), which rules out the pull_requests events and b) because I need the sha of the first commit of the PR and the sha of the latest commit to checkout the PR.

So if anyone with a different view angle could maybe find a quick github webhook API replacement for this, that’d be great. The good thing is that according to jaime-ci-bot/index.js at b506954c4f19b78c3b7f548324d79179daf6b640 · MeFisto94/jaime-ci-bot · GitHub, we don’t really need the check_suite to create the check_run it seems.

Codacy

On a sidenote, we’ve found a third-party/commercial alternative to what we do, which is codacy.
There might be alternatives to codacy so if you can either comment on codacy or have a different provider in mind, please tell us.

Given the much increased feature set compared to what we had, it might be worth considering to use Codacy. It even has support to lint markdown files, and I guess we could also enforce formatting that way, without having to add significant code to the bot otherwise.

The downside is that Codacy does not have “diffing”, which means ever lasting errors of jme might be visible for each new PR, like we need to whitelist false positives, which might be possible.

What are your opinions?
I’d love to finish this ASAP so we can start our “spring cleaning” finally to catch up with what I have in the jme queue.

1 Like
#2

Given the size of our codebase, I’d say that diff-ing is a key requirement. But perhaps it could be tacked onto Codacy somehow.

#3

Was Fix memory leaks in render context #1311 by riccardobl · Pull Request #1408 · jMonkeyEngine/jmonkeyengine · GitHub just a “typo” or do we actually have codacy somewhere already?

Because that would’ve been my other idea, that we’d just try it out.

1 Like
#4

Codacy is running on PRs. I don’t know who initiated that.

1 Like
#5

seems like @RiccardoBlb enabled it.

1 Like
#6

I enabled it some time ago, to try it out. It shouldn’t cause PRs to fail.

1 Like
#7

Take a look at PR 1406. One of the failing tests is labelled “Codacy/PR Quality Review — Not up to standards. This pull request quality could be better.”

#8

Well then that’d be my suggestion, let’s try it out via the Spring Cleaning that I wanted to do anyway, to see how it performs.
It’s analytics also found some things spotbugs could not catch as it applies to native code.

I guess if we can set it up properly to respect formatting and others we get the most value out of it.

@sgold I still have to check the PR, but I guess the “misunderstanding” here is that the fail is only a note to the reviewers, the PR can still be merged

Edit: Yeah, just looking at jmonkeyengine - Codacy - PullRequests, unused imports and stuff

Edit2: It also shows what we might need to configure, it enforces static methods to be camelCase and does not like the underscores in the native methods here.

1 Like
#9

Sure, it could be integrated. But personally, I would never integrate a PR that had failing tests.

#10

It should be completely disabled for PRs now.
You can log in into the codacy organization with your github account and be automatically added to the admins, i think, if it doesn’t work, let me know, i’ll look into doing it manually.
There are some configurations to whitelist “issues” and set the sensitivity/type of analysis, we should look into configuring that.
It is also possible to integrate spotbug running externally (github actions?).

2 Likes
#11

Yeah, jmonkeyengine - Codacy - Files is a good example, it did not detect that we just pass “0” and make it fail in Line 38, it just expects an if around the recursive call.

But then again it’s a good thing it would even catch such cases, in native code.
I’ll look into configuring a bit, but I guess we can’t properly tell until seeing a lot of code/PRs and discussion (e.g. if it was for me, your PRs all need reformatting and optimization of imports etc :wink: )

1 Like
#12

Don’t get me wrong: based on what I’ve seen of Codacy so far, I like it!

It seems to have the required diff-ing built in. And so far, I haven’t disagreed with any of its assessments.

In the specific case of PR 1408, I took the trouble to correct the “not up to standards” aspect of the original PR before integrating.

Once I have more experience with Codacy (on my own open-source repos) I may push to see it re-enabled on the Engine repo.

2 Likes
#13

Speaking of this, we might take the chance to convert jmonkeyengine/JME_style.xml at master · jMonkeyEngine/jmonkeyengine · GitHub into a checkstyle xml config and maybe discuss individual points.

Then it could even comment on spacing, start-imports, lexicographical order of imports, method namings etc.

2 Likes