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

ReviewBot: refactor leaper comment from log functionality. #684

Merged
merged 1 commit into from Feb 15, 2017

Conversation

@jberry-suse
Copy link
Contributor

jberry-suse commented Feb 10, 2017

Simple interface:

  • comment_handler_add(): Add handler to start recording log messages for comment.
  • comment_write(): Write comment from log messages if not similar to previous comment.

Previous implementation had some interesting quirks that I will list in case some were intentional.

  • Checked if self.comment_log: before processing comment even though that will always be set. I changed to do_comment and removed from subsequent functions. Could also move do_comment into ReviewBot, but it seemed more like a guarding flag when turning on the feature rather then something to be part of the API.
  • Leaper.comment_add() provided a result parameter, but it was never used. It was however calculated, but never passed along.
  • Checked state of comment in find_obs_request_comment() and again in add_comment().

Notable changes:

  • Since the bot that made the comment is still important it now is based on bot class name which means the leaper comments went from leaper to Leaper. The code does a case-insensitive check to ensure previous comments are still correlated to leaper.
  • Log message collection is now done by a handler instead of abusing a filter. Filter is still configurable by either adding a filter to handler or setting log level. Defaults to log level logging.INFO as leaper used.
  • comment_write() automatically drop handler which is then required to be added again to provide existing functionality with one less implementation detail exposed. If there is a case where a comment will be posted and another that includes all the lines from the previous comment this will need to be changed, but that seems highly unlikely.
  • The comment marker regular expression supports any state/result instead of fixed list.
  • Caller no longer needs to pass request when calling from inside check_one_request()
  • Caller no longer needs to aggregate log lines unless desired the log handler will be used as default source.

Requested in #641.

@jberry-suse jberry-suse requested a review from lnussel Feb 10, 2017
Simple interface:
- comment_handler_add(): Add handler to start recording log messages
  for comment.
- comment_write(): Write comment from log messages if not similar to
  previous comment.

See leaper.py for example usage.
@jberry-suse jberry-suse force-pushed the jberry-suse:leaper-comment-to-reviewbot branch from 5d6711f to 015d59c Feb 10, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 10, 2017

Coverage Status

Coverage remained the same at 45.346% when pulling 015d59c on jberry-suse:leaper-comment-to-reviewbot into 6eefb42 on openSUSE:master.

Copy link
Member

lnussel left a comment

except for bot name lgtm

def comment_find(self, request=None, state=None, result=None):
"""Return previous comments by current bot and matching criteria."""
# Case-insensitive for backwards compatibility.
bot = self.__class__.__name__.lower()

This comment has been minimized.

Copy link
@lnussel

lnussel Feb 14, 2017

Member

maybe refactor to constructor? it's repeated below but without lower()

This comment has been minimized.

Copy link
@jberry-suse

jberry-suse Feb 14, 2017

Author Contributor

That was intentional since the camelCasing used in class names makes them easier to read if the comment is ever looked at. Extremely minor, but the case here is lower() just like the match group to ensure backwards compatibility with old-style leaper as noted in original description. If desired I can make the new code print lower case as well.

This comment has been minimized.

Copy link
@lnussel

lnussel Feb 15, 2017

Member

ic. I'll merge but I think there should be a follow up commit to have something like self.botname, in case some bot (or instance of one) needs to override this for whatever reason.

This comment has been minimized.

Copy link
@jberry-suse

jberry-suse Feb 15, 2017

Author Contributor

Created #692.

@lnussel lnussel merged commit 5b61b92 into openSUSE:master Feb 15, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jberry-suse jberry-suse deleted the jberry-suse:leaper-comment-to-reviewbot branch Mar 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.