Skip to content
This repository has been archived by the owner on Mar 14, 2023. It is now read-only.

Using commit search to identify new contributors #119

Merged

Conversation

davidalber
Copy link
Collaborator

The current approach to checking for new contributors does not work in repositories with more than 500 contributors because the contributors list API endpoint only associates the first 500 committer emails with GitHub usernames. See #41 for more explanation.

This PR modifies the new contributor check to use the commit search API endpoint. A drawback of this approach is that (at least currently) that endpoint does not provide useful information for fork repositories. To address that, is_new_contributor returns False when the repository is a fork.

To summarize, here are pros and cons:

  • [pro] Repositories with more than 500 contributors (e.g., rust-lang/rust) will no longer treat every PR from repeat contributors, beyond the first 500 contributors, like they are new contributors.
  • [con] Nobody in a fork repository will be treated like a new contributor. I don't think Highfive is currently configured to support any fork repositories, so this won't be noticeable for now. I've asked a question here to see if this is a bug or can be worked around.
  • [con] The Commit Search API is a preview that can change at any time, so there's a risk that this will break one day or just go away altogether. I think this is a better option than what Highfive is currently doing, and if you agree, I am going to follow-up on the PR by adding integration tests so that we will notice through automation if the API changes.

This conflicts with #117, so I'll need to rebase this or that, if one of them is merged. I wanted to get this out now to get commenting going.

The current approach to checking for new contributors does not work
in repositories with more than 500 contributors because the
contributors list API endpoint only associates the first 500
committer emails with GitHub usernames. See rust-lang#41 for more
explanation.

This commit modifies the new contributor check to use the commit
search API endpoint. A drawback of this approach is that (at least
currently) that endpoint does not provide useful information for
fork repositories, which is why the new contributor check has a
condition on whether the repository is a fork.
@davidalber davidalber force-pushed the test-new-contrib-with-commit-search branch from 82727fb to 9636e3c Compare March 25, 2018 08:30
@nrc
Copy link
Member

nrc commented Mar 26, 2018

I think this is a better approach - not working on forks seems like a theoretical problem at this stage

@nrc nrc merged commit 4c6ce3e into rust-lang:master Mar 26, 2018
@davidalber davidalber deleted the test-new-contrib-with-commit-search branch March 27, 2018 03:03
davidalber added a commit to davidalber/highfive that referenced this pull request May 6, 2018
The only usage of `parse_header_links` was removed in rust-lang#119. This
removes the function itself.
davidalber added a commit to davidalber/highfive that referenced this pull request Oct 30, 2018
The 'contributors' key was made obsolete in rust-lang#119, so it is no
longer needed here.
davidalber added a commit to davidalber/highfive that referenced this pull request Dec 6, 2018
Usage of this URL template was removed in rust-lang#119, so it is no longer
needed.
davidalber added a commit that referenced this pull request Dec 6, 2018
Usage of this URL template was removed in #119, so it is no longer
needed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants