[ACHIEVEMENT] The label maker achievemt #38

dunaevsky · 2017-01-26T22:04:15Z

Achievement for putting many labels in a pull request.

Achievement for putting many labels in a pull request

removed double check if labels defined

thatkookooguy · 2017-01-26T22:18:55Z

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

dunaevsky · 2017-01-26T22:25:32Z

Even better

ortichon

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?

👍

ortichon · 2017-01-26T22:24:06Z

achievements/labelBabyJunior.achievement.js

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


please remove the new lines above.

ortichon · 2017-01-26T22:28:09Z

achievements/labelBabyJunior.achievement.js

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


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.

ortichon · 2017-01-26T22:32:28Z

achievements/labelBabyJunior.achievement.js

+	}
+};
+
+function checkIfManyLabels(pullRequest) {


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 ;-)

ortichon · 2017-01-26T22:33:51Z

achievements/labelBabyJunior.achievement.js

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


wrong indentation of }

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

return labels && labels.length > 3;

ortichon · 2017-01-26T22:38:18Z

achievements/labelBabyJunior.achievement.js

+	return false;
+	}
+
+module.exports = labelBabyJunior;


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

dunaevsky · 2017-01-26T22:56:03Z

I did. Changes considered and updated.

func update, and line spacing

ortichon · 2017-01-26T23:30:47Z

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

dunaevsky · 2017-01-26T23:34:16Z

Was lookin for something inline with your style ;)

ortichon · 2017-01-26T23:36:33Z

achievements/labelBabyJunior.achievement.js

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


missing a }

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

It worked until you made me change it =P

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

ortichon · 2017-01-26T23:50:25Z

achievements/labelBabyJunior.achievement.js

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


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

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

ortichon · 2017-01-27T01:14:28Z

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

added brackets, tested, and 6 labels are now minimum

ortichon

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.

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

ortichon · 2017-01-27T13:07:00Z

achievements/labelBabyJunior.achievement.js

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


typo in the word achievement - image does not load

ortichon · 2017-01-27T13:07:57Z

achievements/labelBabyJunior.achievement.js

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


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

ortichon · 2017-01-27T13:08:41Z

achievements/labelBabyJunior.achievement.js

+				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' ,


typo in the word organizing

ortichon · 2017-01-27T13:51:13Z

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 · 2017-01-27T13:56:52Z

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 · 2017-01-27T14:33:14Z

@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 · 2017-01-27T14:34:11Z

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 · 2017-01-27T14:35:48Z

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

ortichon

LGTM
good work

@dunaevsky go ahead and merge this one :)

The label maker achievemt

12c5d1b

Achievement for putting many labels in a pull request

dunaevsky added the achievements issues related to writing or fixing bugs in achievements label Jan 26, 2017

dunaevsky requested a review from thatkookooguy January 26, 2017 22:04

dunaevsky added 2 commits January 27, 2017 00:05

label maker - removed notes

25fec0d

fix 2

66c9165

removed double check if labels defined

thatkookooguy requested review from ortichon and removed request for thatkookooguy January 26, 2017 22:19

ortichon requested changes Jan 26, 2017

View reviewed changes

label maker ach - update 3

346df0c

func update, and line spacing

ortichon requested changes Jan 26, 2017

View reviewed changes

thatkookooguy added this to In Progress in Get ready for 1st Milestone Jan 27, 2017

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

label maker - update 4

21f0980

added brackets, tested, and 6 labels are now minimum

ortichon requested changes Jan 27, 2017

View reviewed changes

label maker typo fixes+image rescale

1bcce5d

thatkookooguy added this to the Get achievibit ready for an official 1st version milestone Jan 27, 2017

label maker image ratio fix+name fix

fe4eb5a

ortichon approved these changes Jan 27, 2017

View reviewed changes

dunaevsky merged commit 2dcacae into master Jan 27, 2017

ortichon deleted the theLabelMaker-achievement branch January 27, 2017 15:13

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ACHIEVEMENT] The label maker achievemt #38

[ACHIEVEMENT] The label maker achievemt #38

dunaevsky commented Jan 26, 2017

thatkookooguy commented Jan 26, 2017

dunaevsky commented Jan 26, 2017

ortichon left a comment

ortichon Jan 26, 2017

ortichon Jan 26, 2017

ortichon Jan 26, 2017

ortichon Jan 26, 2017

ortichon Jan 26, 2017

dunaevsky Jan 26, 2017

ortichon Jan 26, 2017

dunaevsky commented Jan 26, 2017

ortichon commented Jan 26, 2017 •

edited

dunaevsky commented Jan 26, 2017

ortichon Jan 26, 2017

dunaevsky Jan 27, 2017

ortichon Jan 27, 2017

ortichon Jan 26, 2017

dunaevsky Jan 27, 2017

ortichon commented Jan 27, 2017 •

edited

ortichon left a comment •

edited

ortichon Jan 27, 2017 •

edited

ortichon Jan 27, 2017

ortichon Jan 27, 2017

ortichon commented Jan 27, 2017 •

edited

dunaevsky commented Jan 27, 2017

thatkookooguy commented Jan 27, 2017

thatkookooguy commented Jan 27, 2017

thatkookooguy commented Jan 27, 2017 •

edited

ortichon left a comment •

edited

[ACHIEVEMENT] The label maker achievemt #38

[ACHIEVEMENT] The label maker achievemt #38

Conversation

dunaevsky commented Jan 26, 2017

thatkookooguy commented Jan 26, 2017

dunaevsky commented Jan 26, 2017

ortichon 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

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

dunaevsky commented Jan 26, 2017

ortichon commented Jan 26, 2017 • edited

dunaevsky commented Jan 26, 2017

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

Choose a reason for hiding this comment

ortichon commented Jan 27, 2017 • edited

ortichon left a comment • edited

Choose a reason for hiding this comment

Image

Annoying Bug

ortichon Jan 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ortichon commented Jan 27, 2017 • edited

dunaevsky commented Jan 27, 2017

thatkookooguy commented Jan 27, 2017

thatkookooguy commented Jan 27, 2017

thatkookooguy commented Jan 27, 2017 • edited

ortichon left a comment • edited

Choose a reason for hiding this comment

ortichon commented Jan 26, 2017 •

edited

ortichon commented Jan 27, 2017 •

edited

ortichon left a comment •

edited

ortichon Jan 27, 2017 •

edited

ortichon commented Jan 27, 2017 •

edited

thatkookooguy commented Jan 27, 2017 •

edited

ortichon left a comment •

edited