Implementation of bot in this repository

[dhruvmanila]

I'm currently working on implementing a bot: https://github.com/dhruvmanila/algobot
Currently, it can only greet the user who installed the app but I will add the functionality mentioned in this comment soon. The bot uses asynchronous programming in Python, GitHub REST API to make the calls and Heroku for hosting.

If anyone got any further ideas to be implemented or they want to help, please do tell.

P.S. Can anyone suggest a name and display photo for the bot?

[dhruvmanila]

Progress:
dhruvmanila/testing#10

Completed the feature where the bot adds and removes the label when any PR fails or succeeds. Next thing to implement is to write proper tests for it.

[dhruvmanila]

So, the bot is now in a good condition (100% coverage on tests) to be installed, you can check it out at the above link.

@cclauss Do we really want to directly close the PR if it doesn't contain any tests or type hints? Also, this will only work if it contains zero tests and zero type hints.

[poyea]

It's also good for the bot to display those metrics in comments. If the none of the files changed end with .py, that PR MUST be closed. Not sure if it's easy to do for the checking.

[cclauss]

If a contributor suggests a change to one of our .md files or .yml files, I do not think we want to autoclose that PR.

[poyea]

@cclauss yeah I might state that wrongly: sometimes there're PRs with files without any file extension. We can deal with that case

[cclauss]

I think there should be some subtlety in how the bot behaves... If the repo has > 50 open PRs then the bot should be more aggressive about autoclosing non-compliant PRs. If there are few PRs then the bot should add labels to PRs and maintainers should guide the contributors to make appropriate corrections before the stalebot jumps it. Is it possible for the bot to see how many open PRs the repo has?

Should we shorten the waiting times on stalebot?

[dhruvmanila]

This idea can be implemented: auto close the PR with files without any file extension or better to have a list of accepted file extension: .py, .yml or .yaml, .md, .txt

Is it possible for the bot to see how many open PRs the repo has?

Kind of. GitHub REST API (v3) considers all pull requests as issues but not all issues are considered pull requests, so the number we get is the combination of open issues and pull requests. Maybe the GraphQL API (v4) has the ability to give us the exact information, I will have to look into that.

My question was whether to close the PR or label it appropriately when there are no type hints or any kind of tests.

[cclauss]

whether to close the PR or label it appropriately

The bot should always label the PR. My vote would be that if the repo has > 50 open PRs, the bot should also autoclose PR.

[dhruvmanila]

Another thing is that I'm changing the way I commit in the bot repository. I will be making different branches for related features and master will only be used for deployment which is done automatically after all the tests pass. All the constants related to the bot will be in either the utils.py file or const.py file like FAILURE_LABEL, ACCEPTED_PR_FILE_EXTENSION, MIN_PR_FOR_AGGRESSIVE_BOT and so on.

Some ideas when a pull request is opened:

• Check whether the description is empty or not, if it is then close the PR.
• Check whether the user has an open PR and if so then make an appropriate comment and close the PR. MAX_PR_PER_USER=?
• Check for type hints and tests in the submitted files and label it appropriately (close it when there are more number of PRs). I can get the exact number of open PRs via the search API endpoint.
• Check for file extension and close the PR where the file extension is not included in the list. .github/ directory will be ignored.
• After all the checks are completed, label the PR if the tests fail and comment on how to fix the errors.

Also, whatever ideas anyone get just dump it here. I will make it work if it is possible :)

[cclauss]

Check of no boxes are checked in the default commit message.

[dhruvmanila]

Yes, that is a good idea. But should we close the PR or label/comment it appropriately?

[cclauss]

An unmodified default commit message == an empty commit message in my book. I would delete if we have a lot of open PRs.

[dhruvmanila]

Alright, got it

[dhruvmanila]

What are the criteria for marking a pull request as invalid?

• Empty description
• No checkbox ticked
• Invalid file format (Extensionless files)

Any other?

[dhruvmanila]

Woah! Take a look at this: dhruvmanila/testing#15
This is basically what the bot will do:

• Create a PR report about tests, type hints, descriptive parameter names and function names and share it with the user if there are any errors.
• The report will be commented only when the PR is opened and not for the next commits.
• The labels will be added and removed accordingly.

Please open a PR to make the changes for the comments the bot will be posting. I just made the basic ones to test out the bot:

• Labels name: https://github.com/dhruvmanila/algobot/blob/event/pull_request/algobot/constants.py
• Comments: https://github.com/dhruvmanila/algobot/blob/event/pull_request/algobot/pull_requests.py

[dhruvmanila]

With the help of @cclauss I was able to implement the feature where the bot will add a label to indicate the PR is ready to be reviewed by a maintainer (Status: awaiting reviews) if all the checks are passing and the PR satisfies all the requirements. It will be removed once the PR is reviewed.

Code: https://github.com/dhruvmanila/algorithms-keeper/blob/master/algorithms_keeper/pull_requests.py#L268

[dhruvmanila]

It seems that the awaiting reviews label does not need to be removed if there are missing requirements or the checks are failing. If there are any of the labels indicating that some requirements are missing or checks are failing then it is self-explanatory that the PR might not be ready to be reviewed at this moment.

This is my proposed workflow for labeling the status of the pull request:

• PR opened -> Add awaiting reviews label
• Reviewed and changes requested -> Add awaiting changes label
• Approved -> Remove all awaiting ... labels
• New commits after being approved -> Add awaiting reviews label ??

This is how the CPython repository workflow looks like (I have excluded the awaiting core review label as that is not useful for our purposes):
- PR opened -> Add awaiting reviews label
- Reviewed and changes requested -> Add awaiting changes label
- Changes made -> Add awaiting change review label
- Changes requested again -> Add awaiting changes label
- Approved -> Add awaiting merge label

Should we adopt this style of workflow or keep it simple with the previous workflow?
Or adopt only the awaiting change review part of the workflow as that seems to be useful for our purposes.

Ignore this, we don't need this type of workflow.

Another thing I'm thinking is to let the bot comment regarding the missing requirements in every check as just the label part won't be enough for the user to understand. The later comments will include only the content part and not the explanation and relevant links section.

[dhruvmanila]

I have got another idea: instead of commenting, we will make the bot start a check run and then that will initiate the parser in the code and the parser will go through all the files for missing requirements. After that, the bot will annotate the missing requirements right in the code with just one sentence like please provide type hints for 'num' parameter and so on.

[cclauss]

On the labels... keep the flow simple. awaiting review should be a Boolean for tests have passed.

On the comments... make them as helpful and foolproof as possible. The more explicit the comments can be about the requested change, the better.

I would like us to réengage with mypy even if it is for just one directory to start with.

[dhruvmanila]

This is my proposed changes for the label:

• PR open -> Apply awaiting reviews label
• Changes requested -> Apply awaiting changes label
• New changes pushed -> Apply awaiting change review label
• Changes requested again -> Apply awaiting changes label
• Approved -> Remove all awaiting ... label

The bot shouldn't remove the awaiting reviews label if the tests are failing or some require ... labels are present.

As for the comments, how about this?

Another thing I'm thinking is to let the bot comment regarding the missing requirements in every check as just the label part won't be enough for the user to understand. The later comments will include only the content part and not the explanation and relevant links section.

[cclauss]

Please make the choice that you feel most comfortable with. For me, I want one label that I can click on to see all (and only) PRs that are ready to be reviewed. That label tells me that all tests have passed and that changes have been made to address any previous review comments. Having multiple flavors of awaiting review makes the search more complicated for the maintainer.

[dhruvmanila]

For me, I want one label that I can click on to see all (and only) PRs that are ready to be reviewed.

Yes, then it makes sense to remove the awaiting label if the PR does not meet our requirements. Also, sometimes a user would need help in resolving the issues, I will include a line like Tag a maintainer to ask for help

awaiting review => The PR is never reviewed
awaiting change review => The PR was reviewed, changes were requested, user made the changes, needs to be reviewed again.

[dhruvmanila]

PR stages image

Take a look at this.

[dhruvmanila]

On the labels... keep the flow simple. awaiting review should be a Boolean for tests have passed.

Currently, awaiting review is also removed if any of the require ... labels are added.

[dhruvmanila]

Edit: A much simpler workflow then previously suggested: https://imgur.com/74IAXG5
@cclauss Please take a look at this so that I can implement this right away.

[cclauss]

The only thing missing from the flow is the change of status when automated tests fail.

New PR --> Awaiting Review --> automated tests fail --> Awaiting Changes

[dhruvmanila]

This is the workflow around tests are failing label:

New PR -> awaiting review label -> Tests are failing -> Remove the awaiting review label -> Tests are passing -> Add back the awaiting review label.

awaiting changes label is added only when a maintainer requests a change in the PR.

There is tests are failing label which should suggest users to take a look at why they are failing and make the necessary changes. I am thinking about putting the CI logs where there are failures as a comment.

[dhruvmanila]

@cclauss @poyea If possible, please re-approve the PR if there were commits made after being approved.

It makes sense that if the PR is approved at the current state and then there are any changes added the PR needs to be reviewed and approved again. That's the way the bot understands :)

As a safety net I will remove all awaiting... labels after the PR is merged.

[dhruvmanila]

Hi there!

There is one other way the bot could represent the missing requirement information in the pull request:

1. Through Checks API by annotating the code directly. Take a look at this video (scroll down a bit and the first image is the video):

2. Make review comments just like how we do it in the pull request: Fixes #31274 - stream reports to save memory theforeman/foreman#8136 (Scroll down a bit and you will notice it by houndci which is a GitHub app). The status for checks can be seen at the bottom of the pull request where all the checks resides. We can update the status to failure, success accordingly.

[dhruvmanila]

I'm going with option number 2: dhruvmanila/testing#34 (review)

Check run annotation will only be visible in the files section and the checks section which is one click away for the author. We want to make it easy for them to see what is missing in their submission and review comments seems to be the best way. The code is visible, where is it missing is visible, the file name will be provided in the comment.

On a side note, I was rewriting the parser and there's going to be a Visitor class which can visit any node in the tree (source code), and thus, we can perform checks for any rules we define. So, if anyone has other ideas regarding any of the node such as lambda, comprehension, variables, assignments, operators, etc., feel free to dump it here.