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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ACHIEVEMENT] The label maker achievemt #38

Merged
merged 7 commits into from Jan 27, 2017

Conversation

3 participants
@dunaevsky
Copy link
Member

dunaevsky commented Jan 26, 2017

Achievement for putting many labels in a pull request.

The label maker achievemt
Achievement for putting many labels in a pull request

@dunaevsky dunaevsky requested a review from Thatkookooguy Jan 26, 2017

dunaevsky added some commits Jan 26, 2017

fix 2
removed double check if labels defined
@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 26, 2017

@ortichon will review this so you won't get bored of my style 😉

@Thatkookooguy Thatkookooguy requested review from ortichon and removed request for Thatkookooguy Jan 26, 2017

@dunaevsky

This comment has been minimized.

Copy link
Member

dunaevsky commented Jan 26, 2017

Even better

@ortichon
Copy link
Member

ortichon left a comment

Yo,
great work, some nit picks to fix.

I'll check the functionality later today. did you check that the achievement works with the DB?

👍

@@ -0,0 +1,30 @@


This comment has been minimized.

@ortichon

ortichon Jan 26, 2017

Member

please remove the new lines above.

var labelBabyJunior = {
name: 'Label Baby Junior',
check: function(pullRequest, shall) {

This comment has been minimized.

@ortichon

ortichon Jan 26, 2017

Member

please remove this line too :-)

notice that all(all?) other achievements follow the same design rules, It helps to keep organized and maintainable code.
I hope that someday soon we'll have a Lint process to catch and warn on this issues.

}
};

function checkIfManyLabels(pullRequest) {

This comment has been minimized.

@ortichon

ortichon Jan 26, 2017

Member

Its is a common practice to prefix with is<FunctionName> functions that return boolean values.

I think that

if (isManyLabels(pullRequest)) { ...

looks and sounds better ;-)

return labels.length > 3;
}
return false;
}

This comment has been minimized.

@ortichon

ortichon Jan 26, 2017

Member

wrong indentation of }

This comment has been minimized.

@ortichon

ortichon Jan 26, 2017

Member

BTW - rows 24-27 can be shorten to a one liner:

return labels && labels.length > 3;

This comment has been minimized.

@dunaevsky
return false;
}

module.exports = labelBabyJunior;

This comment has been minimized.

@ortichon

ortichon Jan 26, 2017

Member

please add one empty line at the end of the file.
see this answer

@dunaevsky

This comment has been minimized.

Copy link
Member

dunaevsky commented Jan 26, 2017

I did. Changes considered and updated.

label maker ach - update 3
func update, and line spacing
@ortichon

This comment has been minimized.

Copy link
Member

ortichon commented Jan 26, 2017

@dunaevsky Congratulations!!!!! avatar!!!!! ->this is NOT inline comment ;-)

@dunaevsky

This comment has been minimized.

Copy link
Member

dunaevsky commented Jan 26, 2017

Was lookin for something inline with your style ;)

function isManyLabels(pullRequest) {
var labels = pullRequest.labels;
return labels && labels.length > 3;

This comment has been minimized.

@ortichon

ortichon Jan 26, 2017

Member

missing a }

please check that your code is running before you push to Github :-)

This comment has been minimized.

@dunaevsky

dunaevsky Jan 27, 2017

Member

It worked until you made me change it =P

This comment has been minimized.

@ortichon

ortichon Jan 27, 2017

Member

I did noto made you change it, I suggested... :-)


function isManyLabels(pullRequest) {
var labels = pullRequest.labels;
return labels && labels.length > 3;

This comment has been minimized.

@ortichon

ortichon Jan 26, 2017

Member

I think that 4 labels as a minimum is a bit low.
In Kibibit-Code-editor we have at least 4 PRs with 4 labels each.
maybe we should use labels.length > 5 ?
and Kibibit is a small organization with a small team.

Take a look at this PR from AngularJS (Google's Javascript framework) - it has 8(!) labels. They have tons of PRs with 4-5 labels.
LMKWYT

This comment has been minimized.

@dunaevsky

dunaevsky Jan 27, 2017

Member

I agree. 3 is actually just the go to num I use to test multiple stuff.

@ortichon

This comment has been minimized.

Copy link
Member

ortichon commented Jan 27, 2017

@dunaevsky
please change the Pull Request title to [ACHIEVEMENT] The label maker achievemt.

@dunaevsky dunaevsky changed the title The label maker achievemt [ACHIEVEMENT] The label maker achievemt Jan 27, 2017

label maker - update 4
added brackets, tested, and 6 labels are now minimum
@ortichon
Copy link
Member

ortichon left a comment

Image

We should use square images.
so please fix the image aspect ratio to be a square.

Annoying Bug

There is some annoying bug that I'm not sure is your fault, so adding @Thatkookooguy to the loop.

  1. try to create PR, and only after it is already created add 7 labels -> you'll receive the achievement.

  2. try to create PR, but before clicking on "create" add 7 labels and only then create it.
    you won't get the achievement.
    take a look at the logs, there are less labels there than we added.

check: function(pullRequest, shall) {
if (isManyLabels(pullRequest)) {
var achievement = {
avatar : 'images/achievements/theLabelMaker.achievment.jpg',

This comment has been minimized.

@ortichon

ortichon Jan 27, 2017

Member

typo in the word achievement - image does not load

function isManyLabels(pullRequest) {
var labels = pullRequest.labels;
return labels && labels.length > 5;
}

This comment has been minimized.

@ortichon

ortichon Jan 27, 2017

Member

please add a new line separation between the end of the function to the module.exports...

avatar : 'images/achievements/theLabelMaker.achievment.jpg',
name: 'The Label Maker',
short: 'Is this a label maker?',
description: 'You\'ve put many labels, thank you for organizng. You\'re a gift that keeps on re-giving' ,

This comment has been minimized.

@ortichon

ortichon Jan 27, 2017

Member

typo in the word organizing

@ortichon

This comment has been minimized.

Copy link
Member

ortichon commented Jan 27, 2017

Well, looks good now.
we'll wait for @Thatkookooguy's resolution about the labels bug, before approving the PR.

@dunaevsky
BTW - just a thought - don't change it, but let's discuss about it:
you've changed the image to square but did not keep the ratio, so In my eyes, Jerry looks a bit thin now, maybe we should keep the aspect ratio? (the down side is loosing some details from the sides)
what you think?
(it's a question, I just want to hear you opinion)

BTW2 - I've seen that you updated your Github profile with an avatar. If you'll create a Gravatar profile with your Git email, we should see your avatar in Ungit beside your commits too.

@dunaevsky

This comment has been minimized.

Copy link
Member

dunaevsky commented Jan 27, 2017

About, the aspect ratio, you're probably right. I did the easier task.

About the labels bug, I am able to reproduce it. When creating the pull request it only puts 1 label from the 6 added. But then when I try again, and open another pull request, it does add all the labels.

@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 27, 2017

@Kibibit/development about the bug - it worked fine for me. look at your webhook and check if github sent us all the events for the labels. Also check that they all got a status 200 from our server. I can't see your webhook so let me know if you see something fishy

In any case, I'm going to implement this weekend another merge between the pullRequest object we save and the GitHub webhook data once a pull request is merged. That will fix some of the problem of having some data missing regardless of the issue that happen to you @ortichon

@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 27, 2017

I already opened a bug on github about some problems with webhooks. So if you find another one, you can report it to them

@Thatkookooguy

This comment has been minimized.

Copy link
Member

Thatkookooguy commented Jan 27, 2017

I think other then fixing the aspect ratio of the image, IMO this can go in after @ortichon finishes his review

@ortichon
Copy link
Member

ortichon left a comment

LGTM
good work

@dunaevsky go ahead and merge this one :)

@dunaevsky dunaevsky merged commit 2dcacae into master Jan 27, 2017

@ortichon ortichon deleted the theLabelMaker-achievement branch Jan 27, 2017

@Thatkookooguy Thatkookooguy moved this from In Progress to Individual achievements in Get ready for 1st Milestone Feb 2, 2017

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