-
Notifications
You must be signed in to change notification settings - Fork 6
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
skips issue opened/closed event duplicates #294
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tank-bohr Looks good, but need add more test cases.
load_it('github-events', fb) | ||
f = fb.query('(eq what "issue-was-opened")').each.to_a | ||
assert_equal(1, f.length) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tank-bohr Maybe need add test for issue-was-closed
case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tank-bohr I agree with @Yegorov. We need a test for issue-was-closed
case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tank-bohr all good, but I have a small comment
@@ -98,6 +98,14 @@ def self.comments_info(pr) | |||
} | |||
end | |||
|
|||
def self.issue_event_exists?(fact, what) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tank-bohr The self.issue_event_exists?
method checks whether an issue exists or not, but based on the name of the method, it seems that the method is checking for the existence of an event in an issue. What about the name self.issue_exists?
?
@Yegorov What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yegorov I would call it self.issue_seen_already?(fact, what)
, since we are checking the presence of a fact with the given what
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 issue_seen_already?
- readable name.
@Suban05 @yegor256 What do you think if call issue_seen_already?
method after get what
fact, we may not use the last argument in this method. For example:
def self.issue_seen_already?(fact)
Fbe.fb.query(
"(and (eq repository #{fact.repository}) " \
"(eq what #{fact.what}) " \
"(eq issue #{fact.issue}))"
).each.any?
end
# ...
case json[:payload][:action]
when 'closed'
fact.what = 'issue-was-closed'
skip_event(json) if issue_seen_already?(fact)
fact.details = "The issue #{Fbe.issue(fact)} has been closed by #{Fbe.who(fact)}."
when 'opened'
fact.what = 'issue-was-opened'
skip_event(json) if issue_seen_already?(fact)
fact.details = "The issue #{Fbe.issue(fact)} has been opened by #{Fbe.who(fact)}."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yegorov excellent idea! Maybe, we can do this checking at the end of the entire processing of all events? We don't want to have duplicates with any of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 Good point, as an option I think yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yegorov yes, perfect, it looks much better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we set what
before checking the factbase, will the issue_seen_already?
find this fact exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tank-bohr good point. You can do this:
count = Fbe.fb.query("
(agg
(and
(eq repository #{fact.repository})
(eq what #{fact.what})
(eq issue #{fact.issue}))
(count))
).each.to_a.first
You should get a count of facts that satisfy the (and ...)
term. If the count
is larger than 1
, you can do skip_event
.
You can also do a simple Ruby counting:
count = Fbe.fb.query("
(and
(eq repository #{fact.repository})
(eq what #{fact.what})
(eq issue #{fact.issue}))
).each.to_a.count
@@ -98,6 +98,14 @@ def self.comments_info(pr) | |||
} | |||
end | |||
|
|||
def self.issue_event_exists?(fact, what) | |||
Fbe.fb.query( | |||
"(and (eq repository #{fact.repository}) " \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tank-bohr don't forget about where
, it should be set to github
. We may have GitLab, for example, in the same factbase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 But can we handle the events of gitlab
in a file that called github
- judges/github-events/github-events.rb
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yegorov In the github-events
file, we process events only from github, I think it is proposed to put a filter only on events from github like eq where 'github'
, so as not to accidentally receive events from gitlab, for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 updated |
@tank-bohr thanks! |
@Yegorov Hey there! 👋 Great job on the review, although we'd love to see more comments next time. You've snagged +4 points: +8 for the review, -10 for having only 2 comments (we aim for at least 6), and +6 to keep you motivated. Remember, more hits-of-code and comments can boost your bonus up to 32 points! Your current balance is +431. Keep up the good work! 💪 |
@yegor256 Hey there, awesome job on the review! 🎉 You've snagged +8 points, which is pretty sweet considering all the factors at play. Your running balance is now +12. Keep up the great work – remember, more comments and hits-of-code can boost your rewards even higher. Looking forward to your next contribution! |
@tank-bohr Hey there, awesome work on your contribution! 🎉 You've scored a sweet +31 points: 16 for showing up, and an extra 14 for those 147 hits-of-code you cranked out. That's some solid coding! Your balance is now sitting pretty at +120. Keep the momentum going and remember, quality matters just as much as quantity. Looking forward to seeing what you'll bring to the table next! |
fixes #293