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.

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

[ACHIEVEMENT] The Gladiator - achievement for getting all the reaction to a comment #51

Merged
merged 6 commits into from Feb 25, 2017

Conversation

Projects
None yet
4 participants
@dunaevsky
Copy link
Member

dunaevsky commented Jan 27, 2017

The commenter gets the achievement.

-It still does not work for inline comments

-You can also add reactions to the PR itself, I wanted to give an achievement to the creator also, but I didn't see it being audited. @Thatkookooguy LMKWYT

-I am uncertain about the name, so would like suggestions

-Big up and much 馃憤 to @ortichon who helped me de-bug this shit

@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 27, 2017

@dunaevsky opened PR #52 so you'll have the data you want. you can review that one first so you'll have the data available to work in this branch

@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 27, 2017

Also added a data example in PullRequestExample.MD but it's pretty much the same as reactions on comments :-)

@ortichon

This comment has been minimized.

Copy link
Member

ortichon commented Jan 27, 2017

@dunaevsky let me know when you finish all your changes and I'll start reviewing.

@dunaevsky

This comment has been minimized.

Copy link
Member

dunaevsky commented Jan 27, 2017

@ortichon @Thatkookooguy I'ma put a pin on it for tonight and pick up tomorrow sometime, Since there's 2 features to add.

If you have suggestions for the "short" and "description" it will be great.
I like it being called the Gladiator.

@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 27, 2017

@dunaevsky Cool. Just add the help wanted label so we'll know this is not up for review yet and just brainstorming

@dunaevsky

This comment has been minimized.

Copy link
Member

dunaevsky commented Jan 27, 2017

@Thatkookooguy Already have 3 labels since this was my goal here from the get go

@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 28, 2017

Here's how I made a function that gives back all comments (or pull request) creators that got all 6 reactions:

function getCommentAuthorsWithAllReactions(pullRequest) {
    var allComments = _.concat(pullRequest.comments, pullRequest.inlineComments);
    var authors = _.map(allComments, 'author.username');
    var AllCommentsReactions = _.map(allComments, 'reactions');

    // also add pull request description reactions
    authors.push(pullRequest.creator.username);
    AllCommentsReactions.push(pullRequest.reactions);

    getOnlyUniqueReactionsWithoutAuthors(AllCommentsReactions, authors);

    return onlyUsersWithAllReactions(authors, AllCommentsReactions);
}

function reactionsWithoutAuthor(reactions, author) {
    return _.map(_.reject(reactions, ['user.username', author]), 'reaction');
}

function getOnlyUniqueReactionsWithoutAuthors(AllCommentsReactions, authors) {
    _.forEach(AllCommentsReactions, function(reactions, index) {
        AllCommentsReactions[index] =
            _.uniq(reactionsWithoutAuthor(reactions, authors[index]));

    });
}

function onlyUsersWithAllReactions(authors, AllCommentsReactions) {
    var commentAuthorsWithAllReactions = [];

    _.forEach(authors, function(author, index) {
        if (AllCommentsReactions[index].length === 6) {
            commentAuthorsWithAllReactions.push(author);
        }
    });

    return commentAuthorsWithAllReactions;
}

I try and remove all the data that I don't need so that data would be easier to understand.

  1. get all comments (of all types)
  2. from that, create two connected arrays of authors and reactions to that author's comment
  3. add the pull request creator as another author, and the pull request reactions to the reactions array

    Now, the PR data and the comments data are the same. from here, on, the code doesn't know it came from different sources

  4. for each reactions array, remove any reaction by the text author, and remove all the data that doesn't interest us.

    This is done with _map to change every reaction object to the reaction string. At this point, we have all reactions not by the author in string format (example [ "laugh", "+1", "+1" ] ). Finally, we run _.uniq on that array to get only unique values (example: [ "laugh", "+1" ] )

  5. Now, we have exactly the data we want to test. A list of authors we want to test for the achievements, and a list of unique reactions (not by the author). If all reactions were added, it means the unique reactions would be equal to 6 ('+1', '-1', 'laugh', 'confused', 'heart', 'hooray'). So, the final function, just creates an array of the authors that had 6 exactly reactions

So, when you call getCommentAuthorsWithAllReactions(pullRequest), you'll get all the usernames of the users who had something with all reactions but they weren't included in the calculation

getCommentAuthorsWithAllReactions(pullRequest)
// ["dunaevsky", "Thatkookooguy"]
@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 28, 2017

Notice that I don't use _.isEmpty at all. lodash have lots of little tricks to save code from edge cases. For example, you checked that !_.isEmpty(comment.reactions), but if you just did the _.forEach with an empty array or even undefined, lodash will just return undefined

_.forEach(undefined, function(dog) { return console.log('woof') });
_.forEach([], function(dog) { return console.log('woof') });
// both print nothing

Since all lodash function do that, you will just get an empty array if getCommentAuthorsWithAllReactions didn't find reactions at all.

@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 28, 2017

@dunaevsky - So, make the function work and let's start the review.

Please test the achievement (if you already got the achievement in the DB, delete it to test you get it again). Also, test it after you took the latest changes from master into your work
please lint your code with our new eslint file.

you can use eslint inside atom by installing the following packages:

Then, make sure that inside the linter-eslint settings, you check the box image.

This will fix all common mistakes automatically for you when you save the file, and show you errors for things you need to fix manually.

restart atom, open achievibit, and the linter should recognize our settings file automatically and start showing you lint errors

@dunaevsky

This comment has been minimized.

Copy link
Member

dunaevsky commented Jan 28, 2017

@Thatkookooguy cool. I'll go over it. But gonna have to postpone it again

dunaevsky added some commits Jan 27, 2017

[ACHIEVEMENT] used all reactions
This is not including inline comments Yet.
Also, I've noticed there is no collection of reactions for the PR itself. Can we?
The name will change. Also not sure about the beatles.
Used all reactions - add inline and PR check(still problem with PR)
Can't distinguish between when trying to eliminate self reactions.

Also granting to creator is not set yet

@Thatkookooguy Thatkookooguy force-pushed the achiev-all-reactions branch from d95a998 to c6c1ea5 Feb 25, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 25, 2017

Coverage Status

Coverage decreased (-24.5%) to 18.939% when pulling c6c1ea5 on achiev-all-reactions into f6765b2 on master.

@dunaevsky

This comment has been minimized.

Copy link
Member

dunaevsky commented Feb 25, 2017

I did it for the memberberry :*

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 25, 2017

Coverage Status

Coverage increased (+1.4%) to 44.854% when pulling b9e31d6 on achiev-all-reactions into f6765b2 on master.

@Thatkookooguy Thatkookooguy requested review from Thatkookooguy and removed request for ortichon Feb 25, 2017

var achievement = {
avatar: 'images/achievements/gladiator.achievement.gif',
name: 'Gladiator',
short: 'Are You Not Entertained?',

This comment has been minimized.

@Thatkookooguy

Thatkookooguy Feb 25, 2017

Member

E is uppercase

I think a ?! can work here

name: 'Gladiator',
short: 'Are You Not Entertained?',
description: [
'You got all the reactions to your comment. You make everybody feel.'

This comment has been minimized.

@Thatkookooguy

Thatkookooguy Feb 25, 2017

Member

do you like this description? seems kind of too non-descriptive to me

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 25, 2017

Coverage Status

Coverage increased (+1.4%) to 44.854% when pulling 808304d on achiev-all-reactions into f6765b2 on master.

@dunaevsky dunaevsky merged commit 591100a into master Feb 25, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 44.854%
Details

@Thatkookooguy Thatkookooguy deleted the achiev-all-reactions branch Feb 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment