Automated code review for sof

Hi!
I am a member of the team developing monocodus — a service that performs automatic code review of GitHub pull requests to help organizations ensure a high quality of code.

We’ve developed some useful features to the moment, and now we’re looking for early users and feedback to find out what we should improve and which features the community needs the most.

We ran monocodus on a pre-created fork of your repo on GitHub https://github.com/monocodus-demonstrations/sof/pulls, and it found 4 long & overcomplicated functions over 2 pull-requests. I hope that this information will be useful to you and would be happy to receive any feedback here or on my email alena@monocodus.com

If you want to try our service, feel free to follow the link: https://www.monocodus.com
The service is entirely free of charge for open source projects. Hope you’ ll like it :)

[lgirdwood]

@gingerDevilish Thanks, I've installed to evaluate how useful it will be for SOF. we can then make a judgement and decide whether use or not. @thesofproject/steering-committee fyi

[cujomalainey]

Sounds like a great topic for the next TSC meeting

[marc-hb]

http://www.catb.org/esr/writings/taoup/html/ch01s06.html#id2878450

Rule of Silence: When a program has nothing surprising to say, it should say nothing.

Report failures only. Don't spam everyone who submits a small doc fix with a new "Great codestyle!" congratulatory email every time they force-push a typo fix.

[lgirdwood]

@gingerDevilish we are sometimes seeing 2 reviews instead of 1 after a force push.

@lgirdwood Thanks for noting that! Could you provide the link/screenshot showing double reviews?

Also, if you have any suggestions about what kind of suggestions you would like to see and use, we'll be happy to take notes and add this functionality.
We are looking both into adding existing formatters and static analyzers (ex.: clang-tidy) to the system and making them produce user-friendly output, as well as into creating custom checks.

So even if it turns out that it is not very useful as is, it is within your reach to guide us towards what will help you. The goal for our team, for now, is to find opportunities for a real-life test-drive, so that it is visible in practice what is missing and what could be done some other way.

@marc-hb Thanks for a great source — I'll definitely check it out (omg, Unix Philosophy, it's pretty interesting reading even if not for fixing the tool)

Do you mean that if the bot hasn't found any issues in the code, it shouldn't be telling you explicitly there are no issues?

[dbaluta]

Report failures only. Don't spam everyone who submits a small doc fix with a new "Great codestyle!" congratulatory email every time they force-push a typo fix.

I don't mind seeing a check that everything is OK.

[paulstelian97]

@marc-hb Thanks for a great source — I'll definitely check it out (omg, Unix Philosophy, it's pretty interesting reading even if not for fixing the tool)

Do you mean that if the bot hasn't found any issues in the code, it shouldn't be telling you explicitly there are no issues?

It would be preferred if no issues are mentioned like this.

Maybe the bot could add it to the checks just like SOFCI. To note that if the bot is added to have such permissions its reviews will also count towards mergeability.

[marc-hb]

Do you mean that if the bot hasn't found any issues in the code, it shouldn't be telling you explicitly there are no issues?

Yes, it should email failures at most.

Ideally, even emailing failures should be configurable: most statuses and checks in https://github.com/thesofproject/ do not send any email at all, monocodus is almost the first one.

One of the main advantages of post-email tools like github is that they don't dump everything - important stuff and not - in a single inbox with the practical requirement to implement filter to separate jumbled up things again (I may configure a "monocodus -> Trash" email filter soon)

https://help.github.com/en/github/receiving-notifications-about-activity-on-github/getting-started-with-notifications

Ideally, even emailing failures should be configurable: most statuses and checks in https://github.com/thesofproject/ do not send any email at all, monocodus is almost the first one.

One of the main advantages of post-email tools like github is that they don't dump everything - important stuff and not - in a single inbox with the practical requirement to implement filter to separate jumbled up things again (I may configure a "monocodus" -> Trash email filter soon)

https://help.github.com/en/github/receiving-notifications-about-activity-on-github/getting-started-with-notifications

It appears that the reason for that behavior is that the bot uses comments in order to give code review, just like regular users do, and unlike most statuses and checks. This means that if you have notifications about comments in your pull requests enabled, you will get notifications about bot's comments.
(if this is not so, any additional info will be helpful)

Do you think this behavior should also be configurable?

We are looking into ways that would reduce the comment load now (due to the issues you raised, and data we have on hand), and discussing the status checks as a way to deliver the review verdict.
As for email, GitHub claims to be phasing out email notifications by April, 22nd. So we'll likely look into how to make notifications more reasonable as applied to GitHub web notifications interface.

[paulstelian97]

As for email, GitHub claims to be phasing out email notifications by April, 22nd. So we'll likely look into how to make notifications more reasonable as applied to GitHub web notifications interface.

I don't think it's emails that are being phased out, rather there is a new notifications experience on the web notifications page (and how you open notifications and get back to them). Email notifications are not going away. I've been using the new experience in beta and I still get all the emails (and have to mark them as read because I read them from the web interface anyway)

[marc-hb]

It appears that the reason for that behavior is that the bot uses comments in order to give code review, just like regular users do, and unlike most statuses and checks. This means that if you have notifications about comments in your pull requests enabled, you will get notifications about bot's comments. (if this is not so, any additional info will be helpful)

Thanks, yes this is what seems to be happening, it makes sense. I did indeed observe monocodus "spamming" not just my inbox but also the PR page in case of multiple force pushes.

Force pushes are not recommended on github but they map a very common (the most common?) git review model outside github so a number of projects including this one use force pushes.

discussing the status checks as a way to deliver the review verdict.

I think you meant github status OR github check :-)
https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-status-checks#types-of-status-checks-on-github

Either way sounds perfect. As mentioned by others above, it would make monocodus behave like all other similar tools.

I don't think it's emails that are being phased out, rather there is a new notifications experience on the web notifications page (and how you open notifications and get back to them). Email notifications are not going away. I've been using the new experience in beta and I still get all the emails (and have to mark them as read because I read them from the web interface anyway)

Okay, thanks for the corrections. It seems that either way the correct course of action would be to look at more ways to reduce repeating and redundant comments because the notifications are seemingly not going anywhere unless/until we switch to GitHub Checks

Force pushes are not recommended on github but they map a very common (the most common?) git review model outside github so a number of projects including this one use force pushes.

Yes, I am pretty aware of that, and in fact, we use rebase-based workflow in our projects, so force-pushes are ubiquitous for us, as well.
Now, thanks to some of the feedback left in this issue, the team has uncovered a couple of bugs at force-push and merge points, so hopefully, the bot will be doing fewer mistakes and repetitions in the foreseeable future.

JFYI: We have removed the "all good, approving" message on PR approval as redundant for now. Hopefully, there'll be less spam now.

[marc-hb]

There is still one email every time though:

@monocodus[bot] approved this pull request.