Remind first time contributors to add name to CONTRIBUTORS.md #103
Conversation
|
@OmarShehata, thanks for the pull request! Maintainers, we have a signed CLA from @OmarShehata, so you can review this at any time. I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to ✨ ✨ ✨I'm the Cesium Concierge and I'm here to test things. ✨ ✨ ✨ |
There was a problem hiding this comment.
Thanks @OmarShehata! I know this feature will be very useful! I know I have to ask most first time contributors to do this.
Also when you're done, make sure to update the Cesium template to include this check!
README.md
Outdated
| @@ -36,6 +36,7 @@ The possible settings are: | |||
| | `repositories:{full_name}:gitHubToken` | `string` | Token used to verify __outgoing__ requests to GitHub repository | ✓ | |||
| | `repositories:{full_name}:thirdPartyFolders` | `string` | Comma-separated list of folders in which to look for changed files in pull request to remind user to update License. | X | |||
| | `repositories:{full_name}:claUrl` | `string` | The GitHub API URL to the CLA file in JSON form. If undefined it disables CLA checking. See [here](https://developer.github.com/v3/repos/contents/#get-contents) for how the URL should look. _Example:_ https://api.github.com/repos/AnalyticalGraphicsInc/cesium-concierge/contents/specs/data/config/CLA.json | X | |||
| | `repositories:{full_name}:contributorsUrl` | `string` | The GitHub API URL to the `CONTRIBUTORS.md` file in JSON form. If `undefined`, checking for first time contributors will be disabled. | X | |||
There was a problem hiding this comment.
Can we just make this the url relative to the root of the repository? It should be fairly easy to translate this to the GitHub API url.
There was a problem hiding this comment.
I figured I should have it as the API url to be consistent because that's what claUrl is as well. If we change this, would you make the claUrl also relative to the root of the repo?
There was a problem hiding this comment.
Ah, I didn't notice. claUrl is trickier because, at least in our case, it's in a different repo.
I think in this case, we should construct the API url and leave claUrl alone for now.
lib/commentOnOpenedPullRequest.js
Outdated
| var contributorsFileContent = Buffer.from(response.content, 'base64').toString(); | ||
| // Matches items that look like: [<string>](https://github.com/<username>) | ||
| // sets the username as the first capturing group. | ||
| var regex = /\[.+\]\(https:\/\/github.com\/(.*)\)/g; |
There was a problem hiding this comment.
Maybe github.com\/(\w+)? It avoid matching things that aren't part of the username, and will still work with any format as long as they use a valid url.
lib/commentOnOpenedPullRequest.js
Outdated
| // sets the username as the first capturing group. | ||
| var regex = /\[.+\]\(https:\/\/github.com\/(.*)\)/g; | ||
| var match = regex.exec(contributorsFileContent); | ||
| while (defined(match)) { |
There was a problem hiding this comment.
Actually, we already have their username, so can we just check if it matches any of the usernames in this file (rather than matching every username, then checking each)?
There was a problem hiding this comment.
That's a good idea!
lib/templates/pullRequestOpened.hbs
Outdated
| @@ -21,6 +21,11 @@ Can you please send in a [Contributor License Agreement](https://github.com/Anal | |||
| @{{ userName }}, thanks for the pull request! | |||
| {{/if}} | |||
|
|
|||
| {{#if askAboutContributors}} | |||
| Don't forget to add your name to the `CONTRIBUTORS.md` file! | |||
There was a problem hiding this comment.
This seems generic, I think it should be more direct. Something like,
"It looks like you have not added yourself to CONTRIBUTORS.md. Please add yourself and confirm by commenting on this pull request."
| ]); | ||
| } | ||
| if (options.url === contributorsUrl) { | ||
| var content = Buffer.from('* [Jane Doe](https://github.com/JaneDoe)\n* [Boomer Jones](https://github.com/'+userName+')').toString('base64'); |
|
All issues should be resolved now. I plan on adding |
|
Investigating failing tests. |
|
Should be ready now @ggetz ! |
lib/commentOnOpenedPullRequest.js
Outdated
| }) | ||
| .then(function (response) { | ||
| var contributorsFileContent = Buffer.from(response.content, 'base64').toString(); | ||
| var regex = new RegExp('\\[.+\\]\\(https:\\/\\/github.com\\/' + userName + '\\)'); |
There was a problem hiding this comment.
I think we can just look for their username, it doesn't need to necessarily match this format.The simpler it is, the less it may need to change in the future.
|
I've simplified the username check as requested! |
|
Thanks @OmarShehata, looks good to me! Please merge in master. |
|
Resolved the merge conflict, so should be ready to merge now! |
|
Awesome! Let's deploy and update the Cesium template! |
Resolves #84
For this to work in the Cesium repository, we have to add to
contributorsUrlto the Concierge settings in that repository. That url should be:This works by just checking the username of the account that opened the PR and parses
CONTRIBUTORS.mdto check all the usernames in there. A future PR can also abstract this method as a way to tell if this is a first time contributor to display other helpful messages, (as opposed to having to say "If this is your first PR do this" every time).For tests, I added two separate tests to check the cases of when it should post a reminder vs when it should not. Let me know if those should be combined into one test or if this is good! I tested this on a personal private repository and it does correctly post and successfully detects a username in
CONTRIBUTORS.md.