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.

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] Meep Meep #195

Closed
wants to merge 8 commits into from
Closed

[ACHIEVEMENT] Meep Meep #195

wants to merge 8 commits into from

Conversation

ZimGil
Copy link
Member

@ZimGil ZimGil commented Nov 13, 2018

Added the "Meep Meep" achievement

  • New Achievement: "Meep Meep" [resolves [ACHIEVEMENT] Meep Meep #191]

    Will grant an achievement to the author of the first comment within 5 minutes of the Pull Request creation.
    Will work with either comment and inline comments, but not both - only the first

@ghost
Copy link

ghost commented Nov 13, 2018

There were the following issues with this Pull Request

  • Commit: f918e20
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 7df3e9c
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 539f5ed
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: d58800f
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 10173d6
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: d55f0c5
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: c7aa7ce
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 76fd061
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 76fd061 on ZimGil:develop into 16aa683 on Kibibit:develop.

@Kibibit Kibibit deleted a comment from ortichon Nov 14, 2018
@ZimGil
Copy link
Member Author

ZimGil commented Nov 14, 2018

This is actually require changes.
The Achievement should not be granted to the pull request creator, even if he made comments in time.

  • Prevent pull request creator from achieving
  • Make appropriate tests

@ortichon
Copy link
Member

@ZimGil @thatkookooguy
We should address the issue of bots/hound.
do their comments count as comments?
if so, we should filter them out. and actually, update this achievement for each new bot we add?

@thatkookooguy
Copy link
Member

@ZimGil @thatkookooguy
We should address the issue of bots/hound.
do their comments count as comments?
if so, we should filter them out. and actually, update this achievement for each new bot we add?

I'm not sure if we mind bots getting achievements. Just like the greenkeeper bot can get achievements. We don't discriminate bots lol

I'm trying to think if 5 minutes is not too long for just a "first comment" and not a "first review comment". review comments should take time, but regular comments are easy to add and can easily be shorter than 5 minutes

@ortichon
Copy link
Member

I don't have a problem with bots getting achievements too, but then @ZimGil will have to change his logic.
IIRC the current logic gives the achievement to the first (under 5 minutes) commenter.
I guess we'll have to change to logic to give the achievement to ALL under 5 minutes commenters?

@thatkookooguy
Copy link
Member

thatkookooguy commented Nov 14, 2018

I don't have a problem with bots getting achievements too, but then @ZimGil will have to change his logic.
IIRC the current logic gives the achievement to the first (under 5 minutes) commenter.
I guess we'll have to change to logic to give the achievement to ALL under 5 minutes commenters?

I think it should just be shorter. Most bots write a comment based on something that happened.

If the PR format or commit format is wrong, or for a first PR (the welcome bot), the comment will be fast. But for lint errors, tests, coverage, etc., bot comments should be "not as fast".

So we need to test this a bit I guess but it sounds to me like in ver2, the bots that will get the achievements are the welcome-bot, and commitlint-bot. All the other bots won't run fast enough.

Base on what @ZimGil wants, I think we should do one of the two:

  • make this achievement only for "review comments". That's basically a human domain, so if we want to isolate this to humans, that makes sense. Also, a review is hard to do under 5 minutes unless the PR is very small. So I think that makes sense

  • shorten the 5 minutes to something more challenging. writing a normal comment takes nothing. 5 minutes sounds a bit slow IMO for a Road Runner reference :-)
    for normal comments I'll go for something like 1-2 minutes? what do you guys think?

so basically, that depends on what we, and especially @ZimGil , thinks will be more fun\funny when getting this achievement

@thatkookooguy
Copy link
Member

We should maybe test this on a repo with bots installed and see if everything works as expected

@ZimGil
Copy link
Member Author

ZimGil commented Nov 14, 2018

Base on what @ZimGil wants, I think we should do one of the two:

  • make this achievement only for "review comments". That's basically a human domain, so if we want to isolate this to humans, that makes sense. Also, a review is hard to do under 5 minutes unless the PR is very small. So I think that makes sense
  • shorten the 5 minutes to something more challenging. writing a normal comment takes nothing. 5 minutes sounds a bit slow IMO for a Road Runner reference :-)
    for normal comments I'll go for something like 1-2 minutes? what do you guys think?

so basically, that depends on what we, and especially @ZimGil , thinks will be more fun\funny when getting this achievement

I actually think both case fit the reference and are funny.
With that said, I do think review comments are more eligible for this achievement.

@ZimGil ZimGil closed this Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ACHIEVEMENT] Meep Meep
4 participants