Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Leverage all-stars for author de-dupe and additional usernames #8

Merged
merged 3 commits into from
Nov 21, 2015
Merged

Leverage all-stars for author de-dupe and additional usernames #8

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
083985b
check all-stars for each author and maintainer
nexdrew Nov 8, 2015
0cde7c0
update all-stars, use object-assign to merge person data
nexdrew Nov 16, 2015
08efff7
use allStar.subset() to merge only desired properties
nexdrew Nov 20, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
"tmp": "0.0.28"
},
"dependencies": {
"es6-promise": "^3.0.2"
"all-stars": "^1.0.0",
"es6-promise": "^3.0.2",
"object-assign": "^4.0.1"
}
}
23 changes: 19 additions & 4 deletions util/package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,20 @@
'use strict';

var allStars = require( 'all-stars' );
var objectAssign = require( 'object-assign' );

/**
* Check all-stars db for given person and assign to person if found.
*
* @param {Object} person object representing author or maintainer
*
* @return {Object} same object given, possibly modified or augmented
*/
function getAllStar( person ) {
// override properties from all-stars if available
return objectAssign( person, allStars( person ) );
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey hey,

why don't you take the summary result here? :bowtie:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, did you want the "long" or "short" summary? Long looks like:

Rod Vagg <r@va.gg> (npm: rvagg, GitHub: rvagg, Twitter: rvagg)

While short looks like:

Rod Vagg <r@va.gg>

BTW, did you see this comment about assigning? Are you ok returning all that stuff?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rod Vagg r@va.gg (npm: rvagg, GitHub: rvagg, Twitter: rvagg)

The representation thing is something related to the given reporter in credits-cli.
We should provide as much data as possible here without cluttering everything. :)

In a perfect world I'd like to have name, email, npm, github, twitter as strings ( or even array - I don't care too much about that one ).

BTW, did you see this comment about assigning? Are you ok returning all that stuff?

Jup - that's why I am asking for the summary call in all-stars. This will pretty much do the job, without exposing too much data, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'm confused.

My first approach explicitly added string properties npm, github, and twitter to the person object. But it was using a few if statements. You suggested using extend or assign instead.

My second approach used object-assign to implicitly override name and email and add the following properties to the person object:

  • npmUser string
  • githubUser string
  • twitter string
  • id string
  • emails array
  • names array
  • npmUsers array
  • githubUsers array
  • twitters array
  • summary function
  • toString function

Which is probably a bit too much.

You mentioned you'd like the summary result, which could be represented as one of two strings (instead of a function). I asked which string you'd prefer, and I didn't understand your answer.

How many of the properties listed above would you like? Are you suggesting that the summary() function in all-stars return an object (instead of a string) with the following properties?

  • name string
  • email string
  • npm string
  • github string
  • twitter string

And then merge this with the person object via object-assign? Something like this?

var allStar = allStars( person );
return allStar ? objectAssign( person, allStar.summary() ) : person;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really sorry about the confusion. :(

My first approach explicitly added string properties npm, github, and twitter to the person object. But it was using a few if statements. You suggested using extend or assign instead.

Using a lot of if/else statements in a row doesn't look good in my opinion and is not really extendable. Hopefully you agree there?

How many of the properties listed above would you like? Are you suggesting that the summary() function in all-stars return an object (instead of a string) with the following properties?

Aaah I see - here is the confusing coming from. I thought it is returning an object containing strings.
E.g.

{
  name : 'John Doe',
  email : 'john@doe.io',
  npm : 'johndoe',
  github : 'johndow',
  twitter : '@johndoe'
}

var allStar = allStars( person );
return allStar ? objectAssign( person, allStar.summary() ) : person;

This was pretty much what I had in mind, yes. :bowtie:

For all-stars I see 2 requests here ( if you agree ):

  • provide a summary object
  • improve documentation to make return clearer.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I can handle that. I will update. 馃樃 Thanks for working this out with me!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃懐 馃憤


/**
* Parse npm string shorthand into object representation
*
Expand All @@ -10,11 +25,11 @@
function getPersonObject( personString ) {
var regex = personString.match( /^(.*?)\s?(<(.*)>)?\s?(\((.*)\))?\s?$/ );

return {
return getAllStar( {
name : regex[ 1 ],
email : regex[ 3 ],
url : regex[ 5 ]
};
} );
}


Expand All @@ -33,7 +48,7 @@ function getAuthor( packageJson ) {
}

return packageJson.author ?
packageJson.author :
getAllStar( packageJson.author ) :
false;
}

Expand All @@ -54,7 +69,7 @@ function getMaintainers( packageJson ) {
return getPersonObject( maintainer );
}

return maintainer;
return getAllStar( maintainer );
} );
}

Expand Down
35 changes: 35 additions & 0 deletions util/package.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import test from 'ava';
import { authors, index } from 'all-stars';
import packageUtil from './package';

test( 'getAuthor - author is a string', t => {
Expand Down Expand Up @@ -77,6 +78,40 @@ test( 'getAuthor - author is not defined', t => {
t.end();
} );

test( 'getAuthor - author in all-stars has additional properties', t => {
let packageJson = {
author : {
name : 'Archimedes of Syracuse',
email : 'archimedes@syracuse.io',
url : 'https://en.wikipedia.org/wiki/Archimedes'
}
};

// add our fake author to all-stars for mocking purposes
let fakeAuthorId = 'PiDude314159265359';

authors[ fakeAuthorId ] = {
npmUsers : [ fakeAuthorId ],
names : [ packageJson.author.name ],
emails : [ packageJson.author.email ],
githubUsers : [ fakeAuthorId ],
twitters : [ fakeAuthorId ]
};

index[ packageJson.author.name ] = fakeAuthorId;
index[ packageJson.author.email ] = fakeAuthorId;

// test
let author = packageUtil.getAuthor( packageJson );

t.is( author.name, packageJson.author.name );
t.is( author.email, packageJson.author.email );
t.is( author.npmUser, fakeAuthorId );
t.is( author.githubUser, fakeAuthorId );
t.is( author.twitter, fakeAuthorId );
t.end();
} );


test( 'getMaintainers - maintainers is not defined', t => {
let packageJson = {
Expand Down