feat: Add check command that compares GitHub contributors with credited ones #58

machour · 2017-11-03T15:26:38Z

More than often, I find myself having a diff between the numbers of contributors reported by Github, and the contributors actually credited in my .all-contributorsrc.

This PR draft introduce a new CLI command that will fetch contributors from GH api, and run a two-ways diff with the known ones, allowing a project maintainer to easily find who is missing.

Sample output on this repo:

🍺  ~/all-contributors-cli $ ./cli.js check jfmengels/all-contributors-cli
Missing contributors in .all-contributorsrc:
    brycereynolds, chrisinajar, jmeas, spirosikmd, fadc80, snipe
Unknown contributors found in .all-contributorsrc:
    charlike

I still need to paginate the api response if there are more than 100 results, and avoid reporting unknown contributors if their contributions didn't involve coding (video, blog, funding, ..)

I'm also thinking about adding a repository field in the configuration file, to avoid retyping the repo name every time the command is executed.

And I'm not sure if utils/diff.js is the right place for this script 🤔

…data

kentcdodds · 2017-11-03T15:45:43Z

This is super awesome!

avoid reporting unknown contributors if their contributions didn't involve coding (video, blog, funding, ..)

👍

I'm also thinking about adding a repository field in the configuration file, to avoid retyping the repo name every time the command is executed.

You can get that from the projectOwner and projectName fields 👍

kentcdodds

I'm fine with the code as it is 👍 just a couple of things 😄

This is great 👏

kentcdodds · 2017-11-03T15:46:08Z

cli.js

@@ -23,6 +23,8 @@ var argv = yargs
  .usage('Usage: $0 add <username> <contribution>')
  .command('init', 'Prepare the project to be used with this tool')
  .usage('Usage: $0 init')
+  .command('check', 'Compares contributors from Github with the ones credited in .all-contributorsrc')


Github should be GitHub 👍

kentcdodds · 2017-11-03T15:46:29Z

cli.js

@@ -23,6 +23,8 @@ var argv = yargs
  .usage('Usage: $0 add <username> <contribution>')
  .command('init', 'Prepare the project to be used with this tool')
  .usage('Usage: $0 init')
+  .command('check', 'Compares contributors from Github with the ones credited in .all-contributorsrc')
+  .usage('Usage: $0 check <repository>')


Don't forget to remove <repository> when you refactor this to use projectOwner and projectName

kentcdodds · 2017-11-03T15:47:04Z

cli.js

@@ -87,6 +108,9 @@ function promptForCommand(argv) {
    }, {
      name: 'Re-generate the contributors list',
      value: 'generate'
+    }, {
+      name: 'Compare contributors from Github with the credited ones',


Github should be GitHub

machour · 2017-11-03T18:38:22Z

Hi @kentcdodds, thank you for the feedback. I took care of it!

I also implemented API pagination, but it's currently limited to 500 contributors max. I guess that will be more than enough for most projects 😄

For collaborators unknown to GitHub, I added a check to only report those whose contributions includes "code" or "tool". There might be room for improvement here, but I think it's enough for now.

Added some documentation too, you may want to review it as I'm not a native speaker.

And last but not least, I'm not sure how to write tests for this new feature. Aside from testing headers parsing in getNextLink, not really sure what I can do. I would love to have your input on that.

kentcdodds

Super! Could you get rid of the yarn.lock file (maybe add yarn.lock to the gitignore?). Thanks!

kentcdodds · 2017-11-03T19:02:06Z

For collaborators unknown to GitHub, I added a check to only report those whose contributions includes "code" or "tool". There might be room for improvement here, but I think it's enough for now.

Actually, let's make it code or test because tool is often referencing another related project and may or may not be something that happened in the repo. What do you think?

kentcdodds · 2017-11-03T19:04:13Z

Just realized that the yarn.lock file is already in the project. That's fine. This isn't really set up the way my projects are normally. It's @jfmengels project after all 😉

It's also using AVA rather than Jest, so I'm not certain of the best way to test things either, but I think you'll want to mock the request library somehow when testing the check functionality. I'm afraid I don't have too much time to give you feedback or ideas on this point :-/

machour · 2017-11-08T10:55:02Z

@kentcdodds took in account your remarks and added tests using nock() which was already included in the project. 100% coverage for check.js, so if the test suite looks good for you I think I'm done with this PR:
https://github.com/jfmengels/all-contributors-cli/pull/58/files#diff-4c2289f0a9b1e98a126e9406cae8c04b

fixtures are in fixtures/ which is ignored in package.json, the test file uses the .test.js suffix in its name.. I think I won't mess a thing by publishing this. (I'm still a bit new to publishing, triple checking with you 😊 )

kentcdodds

Looking great! Just a few things...

kentcdodds · 2017-11-08T13:36:11Z

cli.js

+
+    var missingInConfig = ghContributors.filter(login => knownContributors.indexOf(login) === -1);
+    var missingFromGithub = knownContributors.filter(login => {
+      return ghContributors.indexOf(login) === -1 && (


Could probably use includes here :)

kentcdodds · 2017-11-08T13:36:18Z

cli.js

+    }, {});
+    var knownContributors = configData.contributors.map(contributor => contributor.login);
+
+    var missingInConfig = ghContributors.filter(login => knownContributors.indexOf(login) === -1);


Could probably use includes here.

kentcdodds · 2017-11-08T13:39:28Z

lib/util/check.test.js

+});
+
+test('Handle a single results page correctly', async t => {
+  await check('jfmengels', 'all-contributors-cli').then(transformed => {


Because you're using await here, you don't need the then you could do:

const transformed = await check ... t.deepEqual(transformed ....

kentcdodds

Sorry, two more things!

kentcdodds · 2017-11-08T15:30:49Z

lib/util/check.js

+    return null;
+  }
+
+  var nextLink = link.split(',').find(s => s.indexOf('rel="next"') > -1);


Could use includes here

kentcdodds · 2017-11-08T15:31:18Z

lib/util/check.js

+}
+
+module.exports = function getContributorsFromGithub(owner, name) {
+  var url = 'https://api.github.com/repos/' + owner + '/' + name + '/contributors?per_page=100';


Could use a template literal:

var url = `https://api.github.com/repos/${owner}/${name}/contributors?per_page=100`;

There you go. Also use template literals for the command output

kentcdodds

Awesome! Thanks!

kentcdodds · 2017-11-08T17:09:26Z

Great work @machour 👏

* docs: update README.md * docs: update .all-contributorsrc

feat: Add checking functionnality that compares contributors with GH …

508f982

…data

machour added enhancement wip labels Nov 3, 2017

kentcdodds reviewed Nov 3, 2017

View reviewed changes

machour added 2 commits November 3, 2017 19:21

fix(check): Use the info from config file, paginate GH data

4a93be5

doc(check): Document the new check command

bf07fba

kentcdodds reviewed Nov 3, 2017

View reviewed changes

machour changed the title ~~feat: Add checking functionality that compares contributors with GH data~~ feat: Add check command that compares contributors with GH data Nov 6, 2017

machour added 3 commits November 6, 2017 12:54

fix(eslint): Add missing semicolon

d417c9b

tests: Add tests for utils.check

82787e1

fix: Check for code and test only

d763ab3

machour removed the wip label Nov 8, 2017

kentcdodds reviewed Nov 8, 2017

View reviewed changes

machour added 2 commits November 8, 2017 16:12

refactor: use includes and correct awaits

7a14d4d

Merge remote-tracking branch 'upstream/master' into diff-functionality

8bad09a

kentcdodds reviewed Nov 8, 2017

View reviewed changes

refactor: more includes and template literals

b566ded

machour changed the title ~~feat: Add check command that compares contributors with GH data~~ feat: Add check command that compares GitHub contributors with credited ones Nov 8, 2017

kentcdodds approved these changes Nov 8, 2017

View reviewed changes

kentcdodds merged commit 88c7a29 into all-contributors:master Nov 8, 2017

machour deleted the diff-functionality branch November 8, 2017 17:23

Berkmann18 pushed a commit that referenced this pull request May 24, 2020

docs: add tbenning as a contributor (#58)

9ef0271

* docs: update README.md * docs: update .all-contributorsrc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add check command that compares GitHub contributors with credited ones #58

feat: Add check command that compares GitHub contributors with credited ones #58

machour commented Nov 3, 2017

kentcdodds commented Nov 3, 2017

kentcdodds left a comment

kentcdodds Nov 3, 2017

kentcdodds Nov 3, 2017

kentcdodds Nov 3, 2017

machour commented Nov 3, 2017

kentcdodds left a comment

kentcdodds commented Nov 3, 2017

kentcdodds commented Nov 3, 2017

machour commented Nov 8, 2017

kentcdodds left a comment

kentcdodds Nov 8, 2017

kentcdodds Nov 8, 2017

kentcdodds Nov 8, 2017

kentcdodds left a comment

kentcdodds Nov 8, 2017

kentcdodds Nov 8, 2017

machour Nov 8, 2017

kentcdodds left a comment

kentcdodds commented Nov 8, 2017

feat: Add check command that compares GitHub contributors with credited ones #58

feat: Add check command that compares GitHub contributors with credited ones #58

Conversation

machour commented Nov 3, 2017

kentcdodds commented Nov 3, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machour commented Nov 3, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Nov 3, 2017

kentcdodds commented Nov 3, 2017

machour commented Nov 8, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Nov 8, 2017