Skip to content
This repository has been archived by the owner on Nov 28, 2018. It is now read-only.

Infer reviewers from recents commits authors #168

Closed
wants to merge 2 commits into from
Closed

Infer reviewers from recents commits authors #168

wants to merge 2 commits into from

Conversation

cyberdelia
Copy link

This stop using blame webpages to find reviewers, and starts using recent top committers on the different files of the PR. This has the advantage to work with 2FA on and requires a lot less code, it also tend to give reviewers that have a overall more "fresher" point of view on the code being changed.

It will fix #147, #166 and #76.

@ghost
Copy link

ghost commented Sep 11, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost ghost added the CLA Signed label Sep 11, 2016
@ghost
Copy link

ghost commented Sep 11, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@StephanBijzitter
Copy link

Is it just me or is it ironic that @mention-bot is not activated for this repository

@waldyrious
Copy link
Contributor

I'm worried about the "stop using blame webpages to find reviewers". Wouldn't this mean that mention-bot would stop producing context-aware reviewer suggestions, based on who has actually worked the code in question?

@vjeux
Copy link
Contributor

vjeux commented Oct 5, 2016

Hey @cyberdelia, sorry for the super late response. I do not think that abandoning the blame is the right approach, but you make an excellent point with "also tend to give reviewers that have a overall more "fresher" point of view on the code being changed".

In the fb-internal version of mention-bot we are now applying a coefficient based on time on every line of code. If the line of code has been written more than 8 months ago, we ignore it entirely. Then, we do a linear interpolation for 8 months being 0 to now being worth 350 points.

This is working much better and I would love to accept a pull request that implements it on this project.

@StephanBijzitter mention-bot is activated on this project. The way it is implemented is that there's a fixed buffer size for the http request and if the pull request you are requesting is bigger than that it crashes and drops the request. This is not ideal and a pull request to make it read whatever has been downloaded within that fixed size would be appreciated.

However, in practice, this only happens for giant pull requests that change everything, which are going to be highly debated and the people involved are going to see it anyway.

I'm going to go ahead and close this pull request as I don't think it's worth losing all the logic. But I'm glad that you were able to use this project as a basis in order to reimplement it in a way that suits your needs and thanks for all the time and efforts you've spent on it!

@vjeux vjeux closed this Oct 5, 2016
@shaneog
Copy link
Contributor

shaneog commented Oct 5, 2016

@vjeux Do you think there will be another way we can use this bot whilst still enforcing 2FA for all organization members?
For me, that was the best part of this PR...

@vjeux
Copy link
Contributor

vjeux commented Oct 5, 2016

We should get GitHub to provide support for accessing the blame information through their API. Here's a random attempt: https://twitter.com/Vjeux/status/783783705722494977

By the way, feel free to use this pull request for your use case, that's the great thing about open source :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2FA Support
6 participants